Photon panelmultiview subviews should be scrollable

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
3 months ago
20 days ago

People

(Reporter: Paolo, Assigned: mikedeboer)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
This makes the subviews unscrollable, and is incorrect:

https://dxr.mozilla.org/mozilla-central/rev/7b8937970f9ca85db88cb2496f2112175fd847c8/browser/themes/shared/customizableui/panelUI.inc.css#356-360

The Developer subview suffers from this issue. It can be observed by moving the panel's anchor to the center of the screen, and I guess it is present on lower resolutions as well.

Updated

3 months ago
Blocks: 1349210
Flags: qe-verify+
Whiteboard: [photon-structure][triage]
(Assignee)

Updated

3 months ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: -- → P1
Comment hidden (mozreview-request)

Updated

3 months ago
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [photon-structure]
(Reporter)

Comment 2

3 months ago
Comment on attachment 8873772 [details]
Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.

I guess you have to try harder than that... :-)

This prevents for example the Web Developer subview form expanding - it seems the Photon sizing code relies on this "bug" in some way.
Attachment #8873772 - Flags: review?(paolo.mozmail) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 4

3 months ago
Comment on attachment 8873772 [details]
Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.

Wring approach. Better luck this week?
Attachment #8873772 - Flags: review?(paolo.mozmail)

Updated

3 months ago
Blocks: 1370584
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
mozreview-review
Comment on attachment 8873772 [details]
Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.

https://reviewboard.mozilla.org/r/145190/#review150608

::: browser/components/customizableui/content/panelUI.css:43
(Diff revision 3)
>    transition-timing-function: ease-out;
>  }
>  
>  /* START photon adjustments */
>  
> +photonpanelmultiview > .panel-viewcontainer:not([transitioning]) {

Discussed this with UX and they're OK with scrolling the contents of the entire panel.
(Reporter)

Comment 7

3 months ago
mozreview-review
Comment on attachment 8873772 [details]
Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.

https://reviewboard.mozilla.org/r/145190/#review150620

Since the "transitioning" attribute is global to the view container, this would likely do the wrong thing if we're transitioning from a scrollable subview to a second level subview.
Attachment #8873772 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 8

3 months ago
(In reply to :Paolo Amadini from comment #7)
> Since the "transitioning" attribute is global to the view container, this
> would likely do the wrong thing if we're transitioning from a scrollable
> subview to a second level subview.

I don't understand. The 'transitioning' attribute is set just before the slide animation start and is removed when it's done animating. At least, in the photon case that's surely how it works. Why would that be problematic for second/ third etc. level views?
Flags: needinfo?(paolo.mozmail)
(Reporter)

Comment 9

3 months ago
Clearing the needinfo request, as we've discussed separately how we can modify the attributes only on the view that is out of sight.
Flags: needinfo?(paolo.mozmail)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

3 months ago
Comment on attachment 8873772 [details]
Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.

Paolo, with this patch I have everything starting to work quite nicely for Photon panels, except for an annoying flicker when moving the panelview node to the offscreen placeholder.
I don't have a clue what's causing that exactly at the moment, so perhaps your experience can help me out a bit here? If you can think of anything, that'd be awesome!

Thanks!
Attachment #8873772 - Flags: review?(paolo.mozmail) → feedback?(paolo.mozmail)
(Reporter)

Comment 12

3 months ago
Well, stepping through shows that when the view is moved to the off-screen container, it is actually visible and placed next to the current view, so there might be something to fix in the off-screen container styling. However, didn't we say that we might be able to keep the view where it currently is inside the viewStack, translated offscreen, by forcing the scrollable area not to be scrollable while we are measuring the total height?
Comment hidden (mozreview-request)
(Assignee)

Comment 14

3 months ago
(In reply to :Paolo Amadini from comment #12)
> However, didn't we say that we might be able to keep the view where it
> currently is inside the viewStack, translated offscreen, by forcing the
> scrollable area not to be scrollable while we are measuring the total height?

I tried that first, but this led to weird rendering issues compared to this relatively clean approach.
I ended up finding inspiration for the fix I just pushed in your code! So this actually solves the issue and makes it relatively clean to implement in the end. I hope you agree and perhaps have suggestions to make it even better/ more efficient!
(Assignee)

Comment 15

3 months ago
Eventually I'll need to refactor this code a bit to not be one big `showSubView` method. But I'd like to save that bit for later.
(Reporter)

Comment 16

3 months ago
mozreview-review
Comment on attachment 8873772 [details]
Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.

https://reviewboard.mozilla.org/r/145190/#review151314

Sounds good. There are two small glitches, one for which we use the cached height even if the anchor was moved in the meantime, and another small one for which the panel briefly extends to touch the border of the screen and then goes back to honor the expected margin, but I think they don't block landing this version.
Attachment #8873772 - Flags: review?(paolo.mozmail) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 18

3 months ago
And yes, it sounds we can refactor this code going forward. It doesn't necessarily matter if we have long functions, but we should use async functions rather than callbacks.
(Assignee)

Comment 19

3 months ago
(In reply to :Paolo Amadini from comment #18)
> And yes, it sounds we can refactor this code going forward. It doesn't
> necessarily matter if we have long functions, but we should use async
> functions rather than callbacks.

I thought about this as well, but Promises resolve after a spin of the event loop, whose mechanics we don't necessarily need. But perhaps that's a premature optimization?
Thinking out loud: there must be a reason why we don't yet have Promise-based event listeners yet...
Blocks: 1369729
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

3 months ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a1615be322c
calculate the size of the panel to be shown off-screen to work around all the panel layout issues. r=Paolo
(Reporter)

Comment 23

3 months ago
(In reply to Mike de Boer [:mikedeboer] from comment #19)
> I thought about this as well, but Promises resolve after a spin of the event
> loop, whose mechanics we don't necessarily need. But perhaps that's a
> premature optimization?

Promise callbacks and async function continuations are invoked at the next microtask, which for a resolved Promise is always before the next spin of the event loop, so this shouldn't be an issue for preventing paint events. Also, all the code inside async functions is fully synchronous until you call "await".

> Thinking out loud: there must be a reason why we don't yet have
> Promise-based event listeners yet...

Complexity of determining completion in the presence of multiple listeners, specifics of re-entrancy, and determining what state the DOM would assume in the meantime.
Backed out for failing browser-chrome's browser_page_action_menu.js with "page-action-multiView" == "page-action-sendToDeviceView":

https://hg.mozilla.org/integration/autoland/rev/58a93f7d6b20b5809ed14294eabccd825aeb174f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8a1615be322cb623d5f4e1dd7d85aae1c6c6b18c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-searchStr=browser-chrome
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105520862&repo=autoland

[task 2017-06-08T15:19:42.446402Z] 15:19:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "page-action-no-devices-button" == "page-action-no-devices-button" - 
[task 2017-06-08T15:19:42.450421Z] 15:19:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "none" == "none" - 
[task 2017-06-08T15:19:42.457545Z] 15:19:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - 
[task 2017-06-08T15:19:42.460100Z] 15:19:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "page-action-sendToDevice-fxa-button" == "page-action-sendToDevice-fxa-button" - 
[task 2017-06-08T15:19:42.468152Z] 15:19:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | "about:preferences#sync" == "about:preferences#sync" - 
[task 2017-06-08T15:19:42.470699Z] 15:19:42     INFO - Leaving test bound sendToDevice_notSignedIn
[task 2017-06-08T15:19:42.472452Z] 15:19:42     INFO - Entering test bound sendToDevice_noDevices
[task 2017-06-08T15:19:42.474259Z] 15:19:42     INFO - TEST-PASS | browser/base/content/test/urlbar/browser_page_action_menu.js | true == true - 
[task 2017-06-08T15:19:42.476008Z] 15:19:42     INFO - Buffered messages finished
[task 2017-06-08T15:19:42.482187Z] 15:19:42     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_page_action_menu.js | "page-action-multiView" == "page-action-sendToDeviceView" - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js :: sendToDevice_noDevices/< :: line 197
[task 2017-06-08T15:19:42.488449Z] 15:19:42     INFO - Stack trace:
[task 2017-06-08T15:19:42.490415Z] 15:19:42     INFO - chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_page_action_menu.js:sendToDevice_noDevices/<:197
Flags: needinfo?(mdeboer)
The test can also time out, most often on OS X: https://treeherder.mozilla.org/logviewer.html#?job_id=105530710&repo=autoland
also backed out from m-c in https://hg.mozilla.org/mozilla-central/rev/8a1615be322c
Comment hidden (mozreview-request)

Comment 28

3 months ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9933f2d4d188
calculate the size of the panel to be shown off-screen to work around all the panel layout issues. r=Paolo
(Assignee)

Updated

3 months ago
Flags: needinfo?(mdeboer)
Out again for frequent timeouts on OSX like https://treeherder.mozilla.org/logviewer.html#?job_id=105835422&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/2abd7ace9f93d1906645a3154bcb18c24bc8d78c
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Flags: needinfo?(mdeboer)

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment hidden (mozreview-request)

Comment 34

2 months ago
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1d75baa49d3
calculate the size of the panel to be shown off-screen to work around all the panel layout issues. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/a1d75baa49d3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Any suggested/recommended STR to be able to verify this? Thanks!
Flags: needinfo?(gijskruitbosch+bugs)

Comment 37

2 months ago
(In reply to Grover Wimberly IV [:Grover-QA] from comment #36)
> Any suggested/recommended STR to be able to verify this? Thanks!

Sure:

1. open nightly

2. move nightly so that the menu button (~roughly the top of the window) is vertically more or less in the middle of the screen, where the screen is vertically small enough that the hamburger panel doesn't entirely fit when opened from here.

3. open hamburger menu (you might see bug 1370584 - in fact, would be good to check that is still an issue!)

4a. open web developer menu in the hamburger menu

4b. open customize mode, move web developer button to the toolbar, exit customize mode, open web developer button.

Expected:
the area with button items scrolls and has a scrollbar

Actual (pre-patch):
the area with button items just gets cut off.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(gwimberly)
Verified on Windows and Linux, but menu is being cut off on Mac OSX.
Flags: needinfo?(gwimberly)

Comment 39

2 months ago
(In reply to Grover Wimberly IV [:Grover-QA] from comment #38)
> Verified on Windows and Linux, but menu is being cut off on Mac OSX.

Hey Grover, I assume this is the same type of cut-off as you reported in bug 1370584 comment 8? If so, I think we should mark this bug as verified. If not, I would like to hear about it! :-)
Flags: needinfo?(gwimberly)
Verified per Comment 39.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Depends on: 1380021
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.