If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 2 bugs)

53 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 verified, firefox57 unaffected)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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).
Flags: qe-verify+
Whiteboard: [photon-structure][triage]
Blocks: 1352692
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [photon-structure]
Comment hidden (mozreview-request)

Updated

4 months ago
QA Contact: gwimberly
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

4 months ago
mozreview-review
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.
Attachment #8875765 - Flags: review?(mdeboer) → review+

Comment 4

4 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/71da3a25b572
switch to photonpanelmultiview for photon, disable off-nightly, r=mikedeboer
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 -
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

4 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/79cbec4c644c
disable panelmultiview in the overflow panel off-nightly for 55, r=mikedeboer
(In reply to Pulsebot from comment #8)
> Pushed by gijskruitbosch@gmail.com:
> 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.
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/79cbec4c644c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1387512
(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!
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)
Verified on 55/56 for Windows/Mac/Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox56: --- → verified
status-firefox57: --- → unaffected
Flags: qe-verify+
Flags: needinfo?(gwimberly)
You need to log in before you can comment on or make changes to this bug.