Closed Bug 1369095 Opened 7 years ago Closed 7 years ago

Photon panelmultiview subviews should be scrollable

Categories

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

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.1 - Jun 26
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: Paolo, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

Bug 1369095 - calculate the size of the panel to be shown off-screen to work around all the panel layout issues.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
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.
Blocks: photon-structure
Flags: qe-verify+
Whiteboard: [photon-structure][triage]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: -- → P1
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [photon-structure]
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 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)
Blocks: 1370584
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.
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)
(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)
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 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)
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?
(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!
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.
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+
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.
(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
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
(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)
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
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
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
Closed: 7 years 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)
(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)
(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.

Attachment

General

Creator:
Created:
Updated:
Size: