1597068 - macOS sheets are no longer anchored to the #navigator-toolbox

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Nguyen :ntim]

Attachment: Before

mozregression gives: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=caf55914ccddba34d462a1206530d7868b6c4992&tochange=72c52c0101cfd102b207224646c3007d6872e65c

The sheet is supposed to be anchored to the toolbox, and this is done using this rule: https://searchfox.org/mozilla-central/rev/492214c05cde6e6db5feff9465ece4920400acc3/browser/themes/osx/browser.css#59

Comment 1

[Tim Nguyen :ntim]

Attachment: After

Comment 2

[Tim Nguyen :ntim]

nsChildView::UpdateThemeGeometries is supposed to take care of this: https://searchfox.org/mozilla-central/rev/492214c05cde6e6db5feff9465ece4920400acc3/widget/cocoa/nsChildView.mm#1788

Comment 3

[Markus Stange [:mstange]]

I think that's the fault of the height: 0 in there. The element needs to have a non-empty rect.

The height was changed from 1px to 0 in https://hg.mozilla.org/mozilla-central/rev/cd43140ba52879d74162066e1531e36ac32df1b8 .

Comment 4

[Markus Stange [:mstange]]

If you have suggestions for a better API, I'd love to hear them. This really doesn't have to be done with -moz-appearance. I bet we can find nicer ways to expose the sheet attachment position to the widget. For example, we could have a special attribute on the toolbox or on an element that defines the "content area". Or we could have an attribute on the root element which contains the ID of the toolbox or of the content area element.

Comment 5

[Tim Nguyen :ntim]

(In reply to Markus Stange [:mstange] from comment #3)

The height was changed from 1px to 0 in https://hg.mozilla.org/mozilla-central/rev/cd43140ba52879d74162066e1531e36ac32df1b8 .

Thanks for looking into it!

(In reply to Markus Stange [:mstange] from comment #4)

For example, we could have a special attribute on the toolbox or on an element that defines the "content area". Or we could have an attribute on the root element which contains the ID of the toolbox or of the content area element.

Either of these suggestions sound good to me.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Moving to backlog as p3 to get it out of the triage list. Please adjust as necessary

Comment 7

[Tim Nguyen :ntim]

[Tracking Requested - why for this release]: Bad regression that needs to be addressed in this release.

Comment 8

[Brendan Dahl [:bdahl]]

Attachment: Bug 1597068 - Revert navigator toolbox sheet anchor changes. r=dao

I'm not exactly sure what changed, but the changes from bug 149582 are no
longer needed to fix test_queryCaretRect.htm and the position of the
drop down dialog.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd4631c7d8cb
Revert navigator toolbox sheet anchor changes. r=dao

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/fd4631c7d8cb

Comment 11

[Cristian Fogel [:cfogel]]

In an attempt to reproduce(for verification), the issue did not appear to reproduce with 72.0a1(2019-11-16) on macOS 10.15.
Any additional details for this?

Comment 12

[Brendan Dahl [:bdahl]]

I'm unsure why this didn't reproduce, but there haven't been any more reports of issues. I'll clear needinfo for now.

Comment 13

[Cristian Fogel [:cfogel]]

Dropping the qe+ flag, since we cannot reproduce the bug.
Fwiw, the issue did not appear during our recurring test sessions on our macOS devices either so the patch appears to be working.

Comment 14

[Cristian Fogel [:cfogel]]

@Tim; any chance you could chime in, if the patch has fixed the issue with the current version(s) for you?

Comment 15

[Tim Nguyen :ntim]

Attachment: Fixed in Nightly

Seems fixed in Nightly for me.

Comment 16

[Tim Nguyen :ntim]

(It's supposed to look like the "Before" screenshot, and the wrong result is the "After" screenshot)