Closed Bug 1451256 Opened 3 years ago Closed 2 years ago

Remove extends from toolbarpaletteitem.

Categories

(Firefox :: General, task, P3)

task

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files, 5 obsolete files)

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 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.
Attachment #8964806 - Flags: review?(dao+bmo) → review-
Comment on attachment 8964806 [details]
Bug 1451256: Remove extends from toolbarpalleteitem.

https://reviewboard.mozilla.org/r/233534/#review239182
Attachment #8964806 - Flags: review?(dao+bmo) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b8b4759b712f
Remove extends from toolbarpalleteitem. r=dao
Summary: Remove extends from toolbarpalleteitem. → Remove extends from toolbarpaletteitem.
Flags: needinfo?(emilio)
Attachment #8974114 - Flags: review?(dao+bmo) → review+
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?
Flags: needinfo?(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.
Flags: needinfo?(emilio)
Priority: -- → P3
(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"?
Flags: needinfo?(emilio)
(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?
Flags: needinfo?(emilio) → needinfo?(paolo.mozmail)
Attachment #8979228 - Flags: review?(dholbert)
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?
Flags: needinfo?(paolo.mozmail) → needinfo?(emilio)
(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?
Flags: needinfo?(emilio)
(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.
Depends on: 1463197
Emilio, if the above looks like the way to go, can you move attachment 8979228 [details] [diff] [review] to bug 1463197?
Attachment #8974114 - Attachment is obsolete: true
Attachment #8974115 - Attachment is obsolete: true
Attachment #8979297 - Attachment is obsolete: true
Attachment #8979297 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8979284 - Attachment is obsolete: true
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)
Attachment #8979228 - Flags: review?(dholbert) → review-
Alright, let's do this then. Paolo, can you double-check this works for you as well?
Attachment #8979228 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
Attachment #8979443 - Flags: review?(dholbert)
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
Flags: needinfo?(paolo.mozmail)
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).
Attachment #8979443 - Flags: review?(dholbert) → review+
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/1dd29cc111db
https://hg.mozilla.org/mozilla-central/rev/2ad915371de0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Emilio, it looks like the CSS change landed in the wrong file (Linux only) compared to the reviewed patch.
Flags: needinfo?(emilio)
Err, messed up while rebasing on top of bug 1450816. Filed bug 1463737
Flags: needinfo?(emilio)
Depends on: 1474142
Type: defect → task
You need to log in before you can comment on or make changes to this bug.