[PDF Ink Signatures] Add size selector dropdown in bottom toolbar [chromium/src : main]
Andy Phan (Gerrit)
Andy Phan added 4 comments![]( "Open in Gerrit")
+rbpotter@ for review, PTAL. I also have some questions listed as comments here, but we can discuss in a separate media if you prefer. Thanks!
<cr-icon-button iron-icon="${this.buttonIcon}"Ideally, the color selector will use ViewerBottomToolbarDropdownElement. The icon should then be a circle with the color that is currently selected (see CrOS Gallery). The color selector doesn't use icons; it's just background color in a circle. Is there a way to make this work with cr-icon-button?
${this.showDropdown_ ? html`<slot></slot>` : ''}The new UI elements are lacking transitions. Will transitions still work using this?
// TODO(crbug.com/369653190): Ideally, the dropdown should be toggled when the
// stroke starts, not when the stroke finishes. Exit out of the dropdown when
// the user draws an ink stroke.There's no click event for when the user clicks the PDF content iframe, so we're relying on the PDF content to respond back about clicking. We might have to add a new plugin message here?
Related details
- Rebekah Potter
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter added 4 comments![]( "Open in Gerrit")
<viewer-bottom-toolbar-dropdown id="size" .buttonIcon="${'pdf:pen-size-3'}">can just use attribute binding since this is a hardcoded string for now. i.e. `button-icon="pdf:pen-size-3"`. The `${` should only be used when data binding.
<cr-icon-button iron-icon="${this.buttonIcon}"Ideally, the color selector will use ViewerBottomToolbarDropdownElement. The icon should then be a circle with the color that is currently selected (see CrOS Gallery). The color selector doesn't use icons; it's just background color in a circle. Is there a way to make this work with cr-icon-button?
Ideally, the color selector will use ViewerBottomToolbarDropdownElement. The icon should then be a circle with the color that is currently selected (see CrOS Gallery). The color selector doesn't use icons; it's just background color in a circle. Is there a way to make this work with cr-icon-button?
I don't think this would work with cr-icon-button as-is. There are 2 different ways to set images for cr-icon-button, see https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/cr_elements/cr_icon_button/cr_icon_button.ts
but neither of these involve just creating a circle with a background color. If you wanted to use cr-icon-button, you could probably create an SVG that looks like a circle, and use iron-icon-fill-color to change its color.
override disconnectedCallback() {
this.tracker_.removeAll();
}if you're resetting in disconnectedCallback(), you should add the event in connectedCallback(). The constructor will only be called once, but connected/disconnected callbacks can be called multiple times if this element is disconnected and reconnected to the DOM.
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">These events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
Related details
- Andy Phan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 5 comments![]( "Open in Gerrit")
<viewer-bottom-toolbar-dropdown id="size" .buttonIcon="${'pdf:pen-size-3'}">can just use attribute binding since this is a hardcoded string for now. i.e. `button-icon="pdf:pen-size-3"`. The `${` should only be used when data binding.
Done
<cr-icon-button iron-icon="${this.buttonIcon}"Rebekah PotterIdeally, the color selector will use ViewerBottomToolbarDropdownElement. The icon should then be a circle with the color that is currently selected (see CrOS Gallery). The color selector doesn't use icons; it's just background color in a circle. Is there a way to make this work with cr-icon-button?
Ideally, the color selector will use ViewerBottomToolbarDropdownElement. The icon should then be a circle with the color that is currently selected (see CrOS Gallery). The color selector doesn't use icons; it's just background color in a circle. Is there a way to make this work with cr-icon-button?
I don't think this would work with cr-icon-button as-is. There are 2 different ways to set images for cr-icon-button, see https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/cr_elements/cr_icon_button/cr_icon_button.ts
but neither of these involve just creating a circle with a background color. If you wanted to use cr-icon-button, you could probably create an SVG that looks like a circle, and use iron-icon-fill-color to change its color.
Ack, thanks.
${this.showDropdown_ ? html`<slot></slot>` : ''}The new UI elements are lacking transitions. Will transitions still work using this?
In this case, the transition is opacity, and fade to full opacity on opening, and zero opacity when closing. I haven't been able to get this to work, and I wonder if it's my CSS or if it's due to the element being removed.
override disconnectedCallback() {
this.tracker_.removeAll();
}if you're resetting in disconnectedCallback(), you should add the event in connectedCallback(). The constructor will only be called once, but connected/disconnected callbacks can be called multiple times if this element is disconnected and reconnected to the DOM.
Thanks, done.
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">These events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
These events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
Related details
- Rebekah Potter
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
2 comments
LGTM but see comment on using 2 way bindings only to set grandparent properties so that the parent is updated via data binding, rather than having the parent set properties directly.
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">Andy PhanThese events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
These events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
I see. This is a bit of an odd way to use these events. To clarify, such events were implemented in CrLitElement as a "compatibility layer" between Lit and Polymer, so that when elements were migrated to Lit any Polymer clients of these elements using 2 way bindings to them would not immediately be broken. 2 way bindings are between parent and child elements.
If the idea is that the parent element needs to react to an event fired by its child so that it can track the change in state, the parent element itself should react to the event. Relying on bubbling the event all the way up to the grandparent, so that the grandparent can then in turn update the parent's state, seems like incorrect layering.
Related details
- Andy Phan
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">Andy PhanThese events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
Rebekah PotterThese events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
I see. This is a bit of an odd way to use these events. To clarify, such events were implemented in CrLitElement as a "compatibility layer" between Lit and Polymer, so that when elements were migrated to Lit any Polymer clients of these elements using 2 way bindings to them would not immediately be broken. 2 way bindings are between parent and child elements.
If the idea is that the parent element needs to react to an event fired by its child so that it can track the change in state, the parent element itself should react to the event. Relying on bubbling the event all the way up to the grandparent, so that the grandparent can then in turn update the parent's state, seems like incorrect layering.
I'm okay with the child responding to and emitting the same event in the parent-child-grandchild hierarchy. It adds a bit more boilerplate to ViewerSidePanelElement and ViewerBottomToolbarElement, but it's not too bad.
I can do this in a separate CL, so temporarily marking as resolved for submission.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rebekah Potter (Gerrit)
Rebekah Potter voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rebekah Potter
| Code-Review | +1 |
1 comment
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">Andy PhanThese events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
Rebekah PotterThese events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
Andy PhanI see. This is a bit of an odd way to use these events. To clarify, such events were implemented in CrLitElement as a "compatibility layer" between Lit and Polymer, so that when elements were migrated to Lit any Polymer clients of these elements using 2 way bindings to them would not immediately be broken. 2 way bindings are between parent and child elements.
If the idea is that the parent element needs to react to an event fired by its child so that it can track the change in state, the parent element itself should react to the event. Relying on bubbling the event all the way up to the grandparent, so that the grandparent can then in turn update the parent's state, seems like incorrect layering.
I'm okay with the child responding to and emitting the same event in the parent-child-grandchild hierarchy. It adds a bit more boilerplate to ViewerSidePanelElement and ViewerBottomToolbarElement, but it's not too bad.
I can do this in a separate CL, so temporarily marking as resolved for submission.
SGTM. There's probably a larger question here of why we need to track identical state through 3 different layers of custom elements. It is indeed common for some initial or backend-created state to flow down from a top level element through several layers, but from that point responsibility for tracking small pieces of the state tends to be managed by subcomponents, and events are only fired up to the top level element when they may be triggering some larger scale change. Part of the benefit of componentization is supposed to be that we don't need to manage every detail of the state in a single complicated element, but instead this can be delegated to subcomponents.
Looking at the PDF viewer code, this problem appears to be created by the fact that unlike most UIs, where browser proxy/backend interfaces are used by many different elements, the PDF viewer only allows interaction with the plugin from the top level component. Wondering if it would be feasible to make the calls to the plugin directly from ViewerBottomToolbar for example, given that it seems to be tracking all the necessary state anyway as of the next CL that passes it the color. The extensions page is an example of a WebUI that passes a browser proxy object ("Service") down through various levels of UI with data bindings; singleton instance is the more common pattern. Not sure which would work better for this case.
Related details
- Andy Phan
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">Andy PhanThese events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
Rebekah PotterThese events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
Andy PhanI see. This is a bit of an odd way to use these events. To clarify, such events were implemented in CrLitElement as a "compatibility layer" between Lit and Polymer, so that when elements were migrated to Lit any Polymer clients of these elements using 2 way bindings to them would not immediately be broken. 2 way bindings are between parent and child elements.
If the idea is that the parent element needs to react to an event fired by its child so that it can track the change in state, the parent element itself should react to the event. Relying on bubbling the event all the way up to the grandparent, so that the grandparent can then in turn update the parent's state, seems like incorrect layering.
Rebekah PotterI'm okay with the child responding to and emitting the same event in the parent-child-grandchild hierarchy. It adds a bit more boilerplate to ViewerSidePanelElement and ViewerBottomToolbarElement, but it's not too bad.
I can do this in a separate CL, so temporarily marking as resolved for submission.
SGTM. There's probably a larger question here of why we need to track identical state through 3 different layers of custom elements. It is indeed common for some initial or backend-created state to flow down from a top level element through several layers, but from that point responsibility for tracking small pieces of the state tends to be managed by subcomponents, and events are only fired up to the top level element when they may be triggering some larger scale change. Part of the benefit of componentization is supposed to be that we don't need to manage every detail of the state in a single complicated element, but instead this can be delegated to subcomponents.
Looking at the PDF viewer code, this problem appears to be created by the fact that unlike most UIs, where browser proxy/backend interfaces are used by many different elements, the PDF viewer only allows interaction with the plugin from the top level component. Wondering if it would be feasible to make the calls to the plugin directly from ViewerBottomToolbar for example, given that it seems to be tracking all the necessary state anyway as of the next CL that passes it the color. The extensions page is an example of a WebUI that passes a browser proxy object ("Service") down through various levels of UI with data bindings; singleton instance is the more common pattern. Not sure which would work better for this case.
In my original prototype, I had ViewerSidePanelElement making calls to the plugin, and not storing fields in PdfViewerElement. Not storing fields in PdfViewerElement caused flickering, so I had to add the params as a property there.
Since the params have already surfaced to PdfViewerElement, I think plugin calls should probably stay there. It's possible to make them in ViewerSidePanelElement/ViewerBottomToolbarElement, but I think the flow would be a bit more confusing, as it's no longer a top-to-bottom or bottom-to-top flow.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[PDF Ink Signatures] Add size selector dropdown in bottom toolbar
Add a size selector dropdown menu and button so that users can choose
brush sizes. Clicking the button will show the dropdown menu with
brush size selection.
To do so, introduce ViewerBottomToolbarDropdownElement to have a
button that opens a dropdown menu. This can be re-used when the color
selector is added in the future.
Low-Coverage-Reason: COVERAGE_UNDERREPORTED This CL adds tests that use
the bottom toolbar element and the new bottom toolbar dropdown element.
- M chrome/browser/pdf/pdf_extension_js_test.cc
- M chrome/browser/resources/pdf/BUILD.gn
- M chrome/browser/resources/pdf/elements/viewer_bottom_toolbar.html.ts
- M chrome/browser/resources/pdf/elements/viewer_bottom_toolbar.ts
- A chrome/browser/resources/pdf/elements/viewer_bottom_toolbar_dropdown.css
- A chrome/browser/resources/pdf/elements/viewer_bottom_toolbar_dropdown.html.ts
- A chrome/browser/resources/pdf/elements/viewer_bottom_toolbar_dropdown.ts
- M chrome/browser/resources/pdf/pdf_viewer.html
- M chrome/browser/resources/pdf/pdf_viewer_wrapper.ts
- M chrome/test/data/pdf/BUILD.gn
- A chrome/test/data/pdf/ink2_bottom_toolbar_dropdown_test.ts
- M chrome/test/data/pdf/ink2_bottom_toolbar_test.ts
Code-Review: +1 by Rebekah Potter
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">Andy PhanThese events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
Rebekah PotterThese events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
Andy PhanI see. This is a bit of an odd way to use these events. To clarify, such events were implemented in CrLitElement as a "compatibility layer" between Lit and Polymer, so that when elements were migrated to Lit any Polymer clients of these elements using 2 way bindings to them would not immediately be broken. 2 way bindings are between parent and child elements.
If the idea is that the parent element needs to react to an event fired by its child so that it can track the change in state, the parent element itself should react to the event. Relying on bubbling the event all the way up to the grandparent, so that the grandparent can then in turn update the parent's state, seems like incorrect layering.
Rebekah PotterI'm okay with the child responding to and emitting the same event in the parent-child-grandchild hierarchy. It adds a bit more boilerplate to ViewerSidePanelElement and ViewerBottomToolbarElement, but it's not too bad.
I can do this in a separate CL, so temporarily marking as resolved for submission.
Andy PhanSGTM. There's probably a larger question here of why we need to track identical state through 3 different layers of custom elements. It is indeed common for some initial or backend-created state to flow down from a top level element through several layers, but from that point responsibility for tracking small pieces of the state tends to be managed by subcomponents, and events are only fired up to the top level element when they may be triggering some larger scale change. Part of the benefit of componentization is supposed to be that we don't need to manage every detail of the state in a single complicated element, but instead this can be delegated to subcomponents.
Looking at the PDF viewer code, this problem appears to be created by the fact that unlike most UIs, where browser proxy/backend interfaces are used by many different elements, the PDF viewer only allows interaction with the plugin from the top level component. Wondering if it would be feasible to make the calls to the plugin directly from ViewerBottomToolbar for example, given that it seems to be tracking all the necessary state anyway as of the next CL that passes it the color. The extensions page is an example of a WebUI that passes a browser proxy object ("Service") down through various levels of UI with data bindings; singleton instance is the more common pattern. Not sure which would work better for this case.
In my original prototype, I had ViewerSidePanelElement making calls to the plugin, and not storing fields in PdfViewerElement. Not storing fields in PdfViewerElement caused flickering, so I had to add the params as a property there.
Since the params have already surfaced to PdfViewerElement, I think plugin calls should probably stay there. It's possible to make them in ViewerSidePanelElement/ViewerBottomToolbarElement, but I think the flow would be a bit more confusing, as it's no longer a top-to-bottom or bottom-to-top flow.
Re-opening as a TODO later.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andy Phan (Gerrit)
Andy Phan added 1 comment![]( "Open in Gerrit")
@current-size-changed="${this.onBrushSizeChanged_}"
@current-type-changed="${this.onBrushTypeChanged_}">Andy PhanThese events aren't fired by default. Either need to set notify: true on these properties in viewer_bottom_toolbar.ts, or manually dispatch such events using fire().
Rebekah PotterThese events are actually fired from ViewerBottomToolbar's children/grandchildren. For instance, InkBrushSelectorElement is a child of ViewerBottomToolbar, and has notify: true on the property [1]. Same with InkSizeSelectorElement.
The flow is PdfViewerElement initializes the values in ViewerBottomToolbar, and consequently, InkBrushSelectorElement. If a user clicks a brush in InkBrushSelectorElement, it notifies PdfViewerElement, which will then set its own property, which sets ViewerBottomToolbar's property.
Andy PhanI see. This is a bit of an odd way to use these events. To clarify, such events were implemented in CrLitElement as a "compatibility layer" between Lit and Polymer, so that when elements were migrated to Lit any Polymer clients of these elements using 2 way bindings to them would not immediately be broken. 2 way bindings are between parent and child elements.
If the idea is that the parent element needs to react to an event fired by its child so that it can track the change in state, the parent element itself should react to the event. Relying on bubbling the event all the way up to the grandparent, so that the grandparent can then in turn update the parent's state, seems like incorrect layering.
Rebekah PotterI'm okay with the child responding to and emitting the same event in the parent-child-grandchild hierarchy. It adds a bit more boilerplate to ViewerSidePanelElement and ViewerBottomToolbarElement, but it's not too bad.
I can do this in a separate CL, so temporarily marking as resolved for submission.
Andy PhanSGTM. There's probably a larger question here of why we need to track identical state through 3 different layers of custom elements. It is indeed common for some initial or backend-created state to flow down from a top level element through several layers, but from that point responsibility for tracking small pieces of the state tends to be managed by subcomponents, and events are only fired up to the top level element when they may be triggering some larger scale change. Part of the benefit of componentization is supposed to be that we don't need to manage every detail of the state in a single complicated element, but instead this can be delegated to subcomponents.
Looking at the PDF viewer code, this problem appears to be created by the fact that unlike most UIs, where browser proxy/backend interfaces are used by many different elements, the PDF viewer only allows interaction with the plugin from the top level component. Wondering if it would be feasible to make the calls to the plugin directly from ViewerBottomToolbar for example, given that it seems to be tracking all the necessary state anyway as of the next CL that passes it the color. The extensions page is an example of a WebUI that passes a browser proxy object ("Service") down through various levels of UI with data bindings; singleton instance is the more common pattern. Not sure which would work better for this case.
Andy PhanIn my original prototype, I had ViewerSidePanelElement making calls to the plugin, and not storing fields in PdfViewerElement. Not storing fields in PdfViewerElement caused flickering, so I had to add the params as a property there.
Since the params have already surfaced to PdfViewerElement, I think plugin calls should probably stay there. It's possible to make them in ViewerSidePanelElement/ViewerBottomToolbarElement, but I think the flow would be a bit more confusing, as it's no longer a top-to-bottom or bottom-to-top flow.
Re-opening as a TODO later.
I believe this is now obsolete after introducing Ink2Manager.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |