1370986 - Overflow panel subview misses anchor styling, cuts off subviews (needs minimum width?)

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

Opening e.g. the history subview, some of its items are cut off. Things like the gecko profiler subview also get cut off.

Furthermore, if you open a subview, unlike in the main app menu, there is no styling that arranges for the user to be able to see the item they clicked with a blue highlight and a 'back' style arrow.

I'm tempted to ifdef out the panelmultiview, especially if that also fixes bug 1370967, and then swap it over to a photonpanelmultiview in the hopes that that will fix some of this stuff for free (and if not, we can fix it properly that way).

Either way, this can't ship to release with 55 in its current state, and it will if we don't do something (this isn't behind the photon structure pref).

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1370986 - disable panelmultiview in the overflow panel off-nightly for 55,

This #ifdefs out the multiview for non-photon-theme, and checks for it being
present in various bits of JS that interact with it. As a result, this will
'fix' the issues in this bug and in bug 1370967 for 55 as it moves off
Nightly. By switching to the photonpanelmultiview, we get proper anchoring
and slightly improved styling (bug 1354086 covers the rest of that), even
where this code *is* enabled. bug 1370967 will still need fixing in
the photonpanelmultiview / webextensions.

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

Comment 2

[:Gijs (he/him)]

I noticed that the summary here says this needs a minimum width. I'm also happy to add that here, though I guess we need some design guidance in that case, and one could argue it's part of bug 1354086.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8875765 [details]
Bug 1370986 - disable panelmultiview in the overflow panel off-nightly for 55,

https://reviewboard.mozilla.org/r/147190/#review151828

LGTM! Thanks.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71da3a25b572
switch to photonpanelmultiview for photon, disable off-nightly, r=mikedeboer

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timing out in browser_photon_customization_context_menus.js:

https://hg.mozilla.org/integration/autoland/rev/ec9d6a6c30ee4f72a279725b882609e18e637100

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=71da3a25b5720039ef7a0c84e0b49a35c86cae0a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105922752&repo=autoland
[task 2017-06-09T19:13:30.836141Z] 19:13:30     INFO - TEST-PASS | browser/components/customizableui/test/browser_photon_customization_context_menus.js | Should be in navbar - 
[task 2017-06-09T19:13:30.838150Z] 19:13:30     INFO - Leaving test bound 
[task 2017-06-09T19:13:30.841528Z] 19:13:30     INFO - Entering test bound 
[task 2017-06-09T19:13:30.844655Z] 19:13:30     INFO - Console message: 1497035430489	addons.xpi	WARN	Attempting to activate an already active default theme
[task 2017-06-09T19:13:30.847053Z] 19:13:30     INFO - TEST-PASS | browser/components/customizableui/test/browser_photon_customization_context_menus.js | new-window-button was found - 
[task 2017-06-09T19:13:30.849555Z] 19:13:30     INFO - Buffered messages logged at 19:11:59
[task 2017-06-09T19:13:30.853190Z] 19:13:30     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2017-06-09T19:13:30.855104Z] 19:13:30     INFO - Buffered messages finished
[task 2017-06-09T19:13:30.857515Z] 19:13:30     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_photon_customization_context_menus.js | Test timed out -

Comment 6

[:Gijs (he/him)]

Comment on attachment 8875765 [details]
Bug 1370986 - disable panelmultiview in the overflow panel off-nightly for 55,

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

Comment 7

[:Gijs (he/him)]

Comment on attachment 8875765 [details]
Bug 1370986 - disable panelmultiview in the overflow panel off-nightly for 55,

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79cbec4c644c
disable panelmultiview in the overflow panel off-nightly for 55, r=mikedeboer

Comment 9

[:Gijs (he/him)]

(In reply to Pulsebot from comment #8)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/79cbec4c644c
> disable panelmultiview in the overflow panel off-nightly for 55, r=mikedeboer

So, the issue that led to the backout is that in some cases, it was possible for the photon panel multiview to cache a height of 0. When running the browser_photon_customization_context_menus.js test, it tried to open the context menu on an item but because the viewContainer had no height, there was nothing to context-click, which broke the test. I didn't see this when writing the patch because I ran the test separately, and in that case it passed.

Specifically, if running the browser/components/customizableui/test/browser_overflow_use_subviews.js test first, where we add, use and then remove an item from the panel, we end up with style="height: 0" cached on the viewContainer of the photonpanelmultiview. This is a bug in the Photon panelmultiview implementation.

I expect it might be fixed by bug 1369095 and friends, but I don't know if that will land in time for 55 branching, and it didn't seem like something we'd bother uplifting given we have no intention of shipping photon with 55. So instead, I verified that the tests passed even when run together, when continuing to use a panelmultiview (which is expected) and then landed the change again.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/79cbec4c644c

Comment 11

[u554753]

(In reply to :Gijs from comment #0)

> Furthermore, if you open a subview, unlike in the main app menu, there is no
> styling that arranges for the user to be able to see the item they clicked
> with a blue highlight and a 'back' style arrow.
> 

Can you elaborate on what this means in terms of verification? Thanks!

Comment 12

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #11)
> (In reply to :Gijs from comment #0)
> 
> > Furthermore, if you open a subview, unlike in the main app menu, there is no
> > styling that arranges for the user to be able to see the item they clicked
> > with a blue highlight and a 'back' style arrow.
> > 
> 
> Can you elaborate on what this means in terms of verification? Thanks!

The fix here disabled photon sturff in 55 and 56. If you put an item that opens a subview in the old hamburger panel (e.g.: bookmarks, history, synced tabs, developer tools) into the overflow panel instead (by having it in the toolbar and making the window narrow), it shouldn't open a subview on 55/56 - either it should have fallback behaviour, or it should open a panel anchored on the overflow chevron button. This bug and fix don't affect 57. Does that help?

Comment 13

[u554753]

Verified on 55/56 for Windows/Mac/Ubuntu.