1451256 - Remove extends from toolbarpaletteitem.

Status: RESOLVED FIXED

(Firefox :: General, task, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Right now it uses extends="xul:button" so that the element it wraps doesn't get mouse events. There's a way to do that with CSS.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1451256: Remove extends from toolbarpalleteitem.

Right now it uses extends="xul:button" so that the element it wraps doesn't get
mouse events. There's a way to do that with CSS, using pointer-events: none on
the child.

Review commit: https://reviewboard.mozilla.org/r/233534/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233534/

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 8964806 [details]
Bug 1451256: Remove extends from toolbarpalleteitem.

https://reviewboard.mozilla.org/r/233534/#review239130

::: browser/base/content/browser.css:1287
(Diff revision 1)
>    display: none;
>  }
>  
> +.toolbarpaletteitem-box {
> +  /* Prevent children from getting events */
> +  pointer-events: none;

This should probably move to xul.css or something so that it works for all toolbar.xml consumers.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964806 [details]
Bug 1451256: Remove extends from toolbarpalleteitem.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233534/diff/1-2/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964806 [details]
Bug 1451256: Remove extends from toolbarpalleteitem.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233534/diff/2-3/

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8964806 [details]
Bug 1451256: Remove extends from toolbarpalleteitem.

https://reviewboard.mozilla.org/r/233534/#review239182

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8b4759b712f
Remove extends from toolbarpalleteitem. r=dao

Comment 7

[Cristian Brindusan [:cbrindusan]]

Backed out changeset b8b4759b712f (bug 1451256) for bc failures on browser_876926_customize_mode_wrapping.js

https://hg.mozilla.org/integration/autoland/rev/1ed7d32adedeec2b72849429339e3e045747f408

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b8b4759b712f0eded56fcd3d271aa0abef540458&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=superseded

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171884816&repo=autoland&lineNumber=3334

Comment 8

[:Paolo Amadini]

Attachment: Bug 1451256 - Remove extends from toolbarpalleteitem.

Right now it uses extends="xul:button" so that the element it wraps doesn't get
mouse events. There's a way to do that with CSS, using pointer-events: none on
the child.

Review commit: https://reviewboard.mozilla.org/r/242404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242404/

Comment 9

[:Paolo Amadini]

Attachment: Bug 1451256 - Dump inner and outer element size.

Review commit: https://reviewboard.mozilla.org/r/242406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242406/

Comment 10

[:Paolo Amadini]

I was eventually able to attribute this failure to the drag-and-drop operation that moves the "toolbarbutton" wrapped in the "toolbarpaletteitem" from the overflow panel to the toolbar. I ran the attached patch with:

./mach test browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js

With "extends", this prints:

Outer: 319x24
Inner: 319x24

Without "extends", this prints:

Outer: 319x24
Inner: 115.26666259765625x24

This means that the "toolbarbutton" is now smaller than the "toolbarpaletteitem". I tried to play with align="stretch" and different box orientations in the binding to no avail.

Emilio, any suggestions?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Does toolbarpaletteitem { display: -moz-box } help?

Button frames extend normal box frames, but don't add much more magic than that. Other than that, I'd try to verify that the sizes are the same flushing, just to double-check.

Comment 12

[:Paolo Amadini]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> Does toolbarpaletteitem { display: -moz-box } help?
> 
> Button frames extend normal box frames, but don't add much more magic than
> that. Other than that, I'd try to verify that the sizes are the same
> flushing, just to double-check.

Ah, I think I figured out what's going on at least in principle, but I don't know what the solution can be without refactoring some of the element structure, which may not be trivial given the complexity of the CustomizableUI code.

The "display" property of the "toolbarpaletteitem" element is ignored in the customization palette because the parent has "display: flex;". Using extends="xul:button", while the "display" value of the "toolbarpaletteitem" element still shows up as "block" in the developer tools, the child of the element stretches correctly as if we were using box layout.

Emilio, again, do you any suggestions on how to move forward? Is there a simple way to achieve the same effect without "extends"?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Don't blockify -moz-box to block.

(In reply to :Paolo Amadini from comment #12)
> Ah, I think I figured out what's going on at least in principle, but I don't
> know what the solution can be without refactoring some of the element
> structure, which may not be trivial given the complexity of the
> CustomizableUI code.
> 
> The "display" property of the "toolbarpaletteitem" element is ignored in the
> customization palette because the parent has "display: flex;". Using
> extends="xul:button", while the "display" value of the "toolbarpaletteitem"
> element still shows up as "block" in the developer tools, the child of the
> element stretches correctly as if we were using box layout.

So, that's per spec, but of course no spec says what -moz-box blockifies to...

> Emilio, again, do you any suggestions on how to move forward? Is there a
> simple way to achieve the same effect without "extends"?

... so we could try this, though not sure if there are parts in the UI that depend on the current behavior, could you give that a shot?

Comment 14

[:Paolo Amadini]

Thanks for the quick reply, this solves the issue with the "toolbarpaletteitem"!

However, it regresses the animatable toolbar buttons, in particular the "position: absolute" in "toolbarbutton-animatable-box". These don't use "display: flex;", but maybe they are influenced by this code in some other way?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Don't apply grid / flex item blockification if display is -moz-box.

(In reply to :Paolo Amadini from comment #14)
> Thanks for the quick reply, this solves the issue with the
> "toolbarpaletteitem"!
> 
> However, it regresses the animatable toolbar buttons, in particular the
> "position: absolute" in "toolbarbutton-animatable-box". These don't use
> "display: flex;", but maybe they are influenced by this code in some other
> way?

Yes, they are. Absolutely positioned items and floats are blockified as well.

See StyleAdjuster::blockify_if_necessary for the conditions that apply:

  https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/servo/components/style/style_adjuster.rs#150

We have special-code to skip the item-based blockification in there... We could skip them for elements with display: -moz-box unconditionally, with something like the attached patch.

However I'm not a complete fan of this, because it introduces the dependency on display from a display adjustment, which may be somewhat problematic / annoying...

Is there any chance to fix the absolutely positioned items instead to use display: block too?

Comment 16

[:Paolo Amadini]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> Is there any chance to fix the absolutely positioned items instead to use display: block too?

This seems to work, thanks. I also noticed another regression related to the sliding animation in the main menu, that I was able to fix in the same way. Since there may be more places where we may have regressions, I'm thinking we can land the first patch and the associated fixes in another bug, for easier tracking if we find out more issues later.

Comment 17

[:Paolo Amadini]

Emilio, if the above looks like the way to go, can you move attachment 8979228 [details] [diff] [review] to bug 1463197?

Comment 18

[:Paolo Amadini]

Attachment: Bug 1451256 - Use block display for the animation boxes in the toolbar and the off-screen view stack.

This is required since the platform code doesn't blockify -moz-box anymore.

Review commit: https://reviewboard.mozilla.org/r/245484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245484/

Comment 19

[:Paolo Amadini]

Comment on attachment 8979284 [details] [diff] [review]
Don't apply grid / flex item blockification if display is -moz-box.

Seems like we may not need this other approach.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8979228 [details] [diff] [review]
Don't blockify -moz-box to block.

Review of attachment 8979228 [details] [diff] [review]:
-----------------------------------------------------------------

Per IRC, I'd rather not make this change, if we can avoid it - it seems like there would be too many side-effects / too much potential for fallout.

(See also bug 579776 comment 9, 10, and 14)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Make toolbarpaletteitem always use box layout.

Alright, let's do this then. Paolo, can you double-check this works for you as well?

Comment 22

[:Paolo Amadini]

Thanks, this seems to work fine. Tryserver build to verify if it fixes the test issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3924fb37d067a78b99e4634f0f17cf6bfeb6e103

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8979443 [details] [diff] [review]
Make toolbarpaletteitem always use box layout.

Review of attachment 8979443 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine, if it works & doesn't break tests (& sounds like that's the case).

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd29cc111db
Make toolbarpaletteitem always use box layout. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad915371de0
Remove extends from toolbarpaletteitem. r=dao

Comment 25

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1dd29cc111db
https://hg.mozilla.org/mozilla-central/rev/2ad915371de0

Comment 26

[:Paolo Amadini]

Emilio, it looks like the CSS change landed in the wrong file (Linux only) compared to the reviewed patch.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Err, messed up while rebasing on top of bug 1450816. Filed bug 1463737