Closed
Bug 979041
Opened 10 years ago
Closed 10 years ago
Make new tab button move at the beginning of the customize mode transition
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
People
(Reporter: mconley, Assigned: alexbardas)
References
Details
(Whiteboard: [Australis:P4])
Attachments
(1 file, 8 obsolete files)
In the default toolbar configuration, when the tabstrip is not overflowed, the new tab button is immediately to the right of the final tab. When we enter customization mode, this button jumps to the end of the toolbar at the end of the transition. We should make this jump occur at the beginning of the transition, if at all possible, to make the end of the transition less jarring.
Comment 1•10 years ago
|
||
Sounds like toolbar items should just be wrapped before the transition rather than after it.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [Australis:P2] → [Australis:P3]
Reporter | ||
Comment 2•10 years ago
|
||
So here's a patch that does the job - but let's see what kind of CART impact it has: Baseline (all but OS X): https://tbpl.mozilla.org/?tree=Try&rev=e9ece32c068f Baseline (OS X): https://tbpl.mozilla.org/?tree=Try&rev=bf8919c4f791 Baseline + Patch: https://tbpl.mozilla.org/?tree=Try&rev=3228bcde168c Compare talos (everyone but OS X): http://compare-talos.mattn.ca/?oldRevs=e9ece32c068f&newRev=3228bcde168c&server=graphs.mozilla.org&submit=true Compare talos (OS X): http://compare-talos.mattn.ca/?oldRevs=bf8919c4f791&newRev=3228bcde168c&server=graphs.mozilla.org&submit=true
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•10 years ago
|
||
Ooof, this just seems to hurt us everywhere. I'm guessing that this is because of the wrapping we're doing. Hrm.
Reporter | ||
Comment 4•10 years ago
|
||
I think we might have to come back to this once we settle down some of our other P1's, P2's and P3's.
Whiteboard: [Australis:P3] → [Australis:P4]
Comment 5•10 years ago
|
||
There seems to be some interaction here with the phenomenon described under Bug 983032, in that the behavior described here (the positioning of the "Make New Tab" Icon during ToolBar Customization) seems to vary with the placement of the "List All Tabs" Icon. To wit: (A) Start with a New Window with only one Tab and the "List All Tabs" Icon placed on the left of the Tabs bar. The "Make New Tab" Icon is immediately to the right of the Tab. Activate Customization: the "Make New Tab" Icon jumps all the way to the end -- the far right -- of the bar. Close Customization and it returns to the immediate right of the Tab. (B) Now, activate Customization and move the "List All Tabs" Icon to the far right of the bar, but to the immediate left of the "Make New Tab" Icon, then close Customization. The "List All Tabs" Icon disappears, but the "Make New Tab" Icon remains on the far right of the bar. (C) Again, activate Customization. The "List All Tabs" Icon re-appears. Drag the "List All Tabs" Icon to the extreme right of the bar, to the right of the "Make New Tab" Icon; the "Make New Tab" Icon jumps back to its former place at the immediate right of the Tab. (D) Finally, activate Customization and move the "List All Tabs" Icon to anywhere left of the middle of the Tabs bar; it will land on the far left and remain visible after you close Customization.
Comment 6•10 years ago
|
||
Step (C): __After you close Customization__ the "Make New Tab" icon jumps ... Also, the "List All Tabs" Icon disappears.
Comment 7•10 years ago
|
||
To justify the backlog+, this is currently the biggest source of jumpiness in the customization transition.
Flags: firefox-backlog+
Updated•10 years ago
|
Summary: Make new tab button move at the beginning of the customize mode transition → [UX] Make new tab button move at the beginning of the customize mode transition
Whiteboard: [Australis:P4] → [Australis:P4] [ux] p=0
Updated•10 years ago
|
Whiteboard: [Australis:P4] [ux] p=0 → [Australis:P4] [ux] p=0 [qa-]
Comment 8•10 years ago
|
||
This is engineering work IMO. Mikes patch already did the right thing from a user perspective but caused some regressions. Marco, if you see UX work to be done for this bug, perhaps it would be better to file a new bug about it (since this one already has a patch).
Summary: [UX] Make new tab button move at the beginning of the customize mode transition → Make new tab button move at the beginning of the customize mode transition
Whiteboard: [Australis:P4] [ux] p=0 [qa-] → [Australis:P4] p=0 [qa-]
Comment 9•10 years ago
|
||
Hi Philipp, thanks for the update. I'm going to 'need info' Madhava to see if a follow up UX bug needs to be filed. He originally added this bug as a UX one.
Flags: needinfo?(madhava)
Comment 10•10 years ago
|
||
Happy to be wrong and have this be ready for engineering. Let's keep this in the priority list as an ENG bug.
Flags: needinfo?(madhava)
Comment 11•10 years ago
|
||
Thanks Madhava, stays on priority list as an ENG bug.
Updated•10 years ago
|
QA Whiteboard: [qa-]
Whiteboard: [Australis:P4] p=0 [qa-] → [Australis:P4]
Updated•10 years ago
|
Iteration: --- → 34.1
Updated•10 years ago
|
Assignee: mconley → nobody
Iteration: 34.1 → ---
Updated•10 years ago
|
Status: ASSIGNED → NEW
Updated•10 years ago
|
Points: --- → 8
Assignee | ||
Comment 12•10 years ago
|
||
I'll first try Mike's patch and re-run the tests to see if anything has changed since then.
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•10 years ago
|
||
I've rerun the tests with Mike's initial patch and things haven't changed much (the same performance problems). I've tested more than 10 different scenarios and I saw where we can achieve some improvements. Given that we don't actually need to wrap everything before the transition starts, I've added only the AREA_TABSTRIP to be wrapped before. The new performance tests are: http://compare-talos.mattn.ca/?oldRevs=64a3c2cbe287&newRev=cea08c67e9e1&server=graphs.mozilla.org&submit=true (linux + windows, mac is almost the same, I have it in other commits). I can submit more of them, right now this is the one I find most relevant. They are a bit worse than without the patch applied, but better than the first patch. I think we can further improve it without major changes in the code base: we don't actually need that initial wrap before the transition. We can only display the '+' before the transition starts and do the wrap after the transition (the user can't interact with the page before it finishes). Thoughts?
Attachment #8471884 -
Flags: review?(mconley)
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8471884 [details] [diff] [review] WIP Patch 2: Improved performance from patch #1 Review of attachment 8471884 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/CustomizeMode.jsm @@ +972,5 @@ > let window = this.window; > // Add drag-and-drop event handlers to all of the customizable areas. > return Task.spawn(function() { > this.areas = []; > + let areas = area ? [area] : CustomizableUI.areas; The redefinition of area (once as the function param, again in the for-of), is a little confusing. We might want to rename that. Also, the distinction between this.areas and the local areas is a little fuzzy. Basically, there are too many "area(s)" in 975-976, imo. :) While I'm here though - isn't there the bigger problem of repeated wrapping of the TabsToolbar? When we call _wrapToolbarItems with no parameters the next time, won't TabsToolbar be wrapped again, or am I forgetting how this works?
Attachment #8471884 -
Flags: review?(mconley)
Assignee | ||
Comment 15•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6d35378cf250
Assignee | ||
Comment 16•10 years ago
|
||
I've refactored the previous patch and now an area is wrapped only once. I'll rerun the Talos tests shorlty, but I don't expect any improvements in performance from my previous patch.
Attachment #8471884 -
Attachment is obsolete: true
Attachment #8473984 -
Flags: review?(mconley)
Assignee | ||
Comment 17•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a95da768b716
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8473984 -
Attachment is obsolete: true
Attachment #8473984 -
Flags: review?(mconley)
Attachment #8474038 -
Flags: review?(mconley)
Updated•10 years ago
|
Iteration: --- → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Reporter | ||
Comment 19•10 years ago
|
||
Hey Alex, I haven't forgotten about this - just had a lot on my plate recently. I'll have a review for you today.
Assignee | ||
Comment 20•10 years ago
|
||
No problem, Mike. The last patch has the performance as my previous patch. Unfortunately, adding that toolbar item before the transition seems to affect a little the talos tests, but not necessarily the perceived performance. Locally, I made sure that mochitests are ok so that no other functionality is broken by this fix.
Reporter | ||
Comment 21•10 years ago
|
||
Comment on attachment 8474038 [details] [diff] [review] Patch 3: Improved performance from patch #1 Review of attachment 8474038 [details] [diff] [review]: ----------------------------------------------------------------- Again, I like the approach, but I think I want to see something slightly different - I want to see if we can avoid iterating over all of the children of the tabstrip when _wrapToolbarItems is called. Also, while I trust you're looking at the talos numbers, I would prefer if you could post the compare-talos link for your patch. Here's how you do that: 1) Push a try run that runs the CART Talos test without your patch. We will call this the "baseline" push. 2) Push a second try run that runs the CART Talos test _with_ your patch. 3) Fill in the blanks here: http://compare-talos.mattn.ca/ where "Original revision 1" will be the commit ID for the baseline push, and "New revision" will be the commit ID for the patch push. Click "compare", and wherever it redirects you, copy that link. 4) Put links to the baseline try push, patch try push, and the compare-talos in a comment in this bug. 5) Trigger a few extra runs of CART on both the baseline and patch pushes. I go for about 5 total each. Once the CART runs complete, the compare-talos link (after a little wait) will allow us to see if there's a statistically significant regression or improvement in CART scoring, and allow us to break down in which CART subtest the regression or improvement is occurring. Let me know if you have questions about any of that. Thanks again for working on this. :) ::: browser/components/customizableui/CustomizeMode.jsm @@ +218,5 @@ > let customizer = document.getElementById("customization-container"); > customizer.parentNode.selectedPanel = customizer; > customizer.hidden = false; > > + yield this._wrapToolbarItem(CustomizableUI.AREA_TABSTRIP); Why are we not adding drag and drop handlers for the tabstrip? @@ +976,5 @@ > + if (aAddDragHandler) { > + this._addDragHandlers(target); > + } > + > + for (let child of target.children) { This is going to run twice for the tabstrip, though we will fail the isWrappedToolbarItem check, and not wrap anything... still, I should think that calling wrapToolbarItem on something that's been wrapped would be a no-op. Notice how when we call _wrapToolbarItems, we create this array of areas, and once we wrap, we add the area to the areas array. Perhaps we could make it so that _wrapToolbarItems adds to the areas array, and bails early if aArea is already in the array?
Attachment #8474038 -
Flags: review?(mconley)
Assignee | ||
Comment 22•10 years ago
|
||
Thank you for those details Mike. I also added a link with some comparisons in comment 13 and was referring to it in my previous comments. So, I refactored the code in order to not iterate through an element's children the second time, but I had to use a Map instead of an Array in order to make the lookup efficient (we don't care about the order so we don't actually need an array anyways). Take notice that we have to add the actual elements as keys, not the areas. I have the following results: The baseline push: https://tbpl.mozilla.org/?tree=Try&rev=9891d47e6ed2 1. This is the basic refactoring https://tbpl.mozilla.org/?tree=Try&rev=1311cb764491 http://compare-talos.mattn.ca/?oldRevs=9891d47e6ed2&newRev=1311cb764491&server=graphs.mozilla.org&submit=true 2. I've added here a setTimeout of 0 before adding the attributes at the beginning of the transition https://tbpl.mozilla.org/?tree=Try&rev=1c930906654d http://compare-talos.mattn.ca/?oldRevs=9891d47e6ed2&newRev=1c930906654d&server=graphs.mozilla.org&submit=true 3. I've replaced the setTimeouts for the beginning / end of the transition with requestAnimationFrame https://tbpl.mozilla.org/?tree=Try&rev=da1c5ba48300 http://compare-talos.mattn.ca/?oldRevs=9891d47e6ed2&newRev=da1c5ba48300&server=graphs.mozilla.org&submit=true As you can see, adding an extra setTimeout doesn't improve the initial case too much, but the requestAnimationFrame is much better.
Attachment #8474038 -
Attachment is obsolete: true
Attachment #8476752 -
Flags: review?(mconley)
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8476752 [details] [diff] [review] Patch 4: Wrap an area (area_tabstrip) before the transition and use requestAnimationFrame Alex was going to try to drill in to the CART performance regressions being reported for his patch, so unmarking for review.
Attachment #8476752 -
Flags: review?(mconley)
Assignee | ||
Comment 24•10 years ago
|
||
The baseline push: https://tbpl.mozilla.org/?tree=Try&rev=5171ca153dff The patch: https://tbpl.mozilla.org/?tree=Try&rev=0ef1fcdef205 http://compare-talos.mattn.ca/?oldRevs=5171ca153dff&newRev=0ef1fcdef205&server=graphs.mozilla.org&submit=true The initial performance was improved.
Attachment #8476752 -
Attachment is obsolete: true
Attachment #8480248 -
Flags: review?(mconley)
Assignee | ||
Comment 25•10 years ago
|
||
I'd say that the performance was dramatically improved for case 3-customize-enter-css.error (in general *.error). The previous patch (with requestAnimationFrame on entering) improved the performance for the other cases.
Assignee | ||
Comment 26•10 years ago
|
||
Removed a leftover, unused method from the last patch.
Attachment #8386384 -
Attachment is obsolete: true
Attachment #8480248 -
Attachment is obsolete: true
Attachment #8480248 -
Flags: review?(mconley)
Attachment #8480314 -
Flags: review?(mconley)
Comment 27•10 years ago
|
||
Comment on attachment 8480314 [details] [diff] [review] Synchronously wrap an area (area_tabstrip) before the transition and use requestAnimationFrame instead of setTimeout in doTransition > // Bug 962677: We let the event loop breathe for before we do the final > // stage of the transition to improve perceived performance. >- this.window.setTimeout(function () { >+ this.window.requestAnimationFrame(() => { Please update that comment.
Assignee | ||
Comment 28•10 years ago
|
||
Here is a set of links, for multiple platforms: Baseline: XP: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/abardas@mozilla.com-5171ca153dff/try-win32/firefox-34.0a1.en-US.win32.zip OS X: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/abardas@mozilla.com-5171ca153dff/try-macosx64/firefox-34.0a1.en-US.mac.dmg Builds with the patch applied: XP: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/abardas@mozilla.com-0ef1fcdef205/try-win32/firefox-34.0a1.en-US.win32.zip OS X: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/abardas@mozilla.com-0ef1fcdef205/try-macosx64/firefox-34.0a1.en-US.mac.dmg Philipp, can you take a look and see if they do what they should and if the performance is good? From our tests it should be very ok, but we need an UX expert advice :-)
Flags: needinfo?(philipp)
Reporter | ||
Comment 29•10 years ago
|
||
Comment on attachment 8480314 [details] [diff] [review] Synchronously wrap an area (area_tabstrip) before the transition and use requestAnimationFrame instead of setTimeout in doTransition Review of attachment 8480314 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable, and nice usage of Maps instead of that Array! See my notes below, and then let's see what phlsa and avih have to say about the perceived performance of your build. ::: browser/components/customizableui/CustomizeMode.jsm @@ +73,5 @@ > _transitioning: false, > window: null, > document: null, > // areas is used to cache the customizable areas when in customization mode. > + areas: new Map(), Because this is set on the prototype, it means that the same areas is going to be shared across every instance of CustomizeMode - so that means each window will share this. We probably don't want that - we should assign the new Map to areas in the constructor. @@ +222,5 @@ > let customizer = document.getElementById("customization-container"); > customizer.parentNode.selectedPanel = customizer; > customizer.hidden = false; > > + this._wrapToolbarItem(CustomizableUI.AREA_TABSTRIP, true); I think I would prefer the older approach of having two functions _wrapToolbarItem and _wrapToolbarItemSync, since the "true" parameter really doesn't tell us much about what this is doing.
Attachment #8480314 -
Flags: review?(mconley)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8480314 -
Attachment is obsolete: true
Updated•10 years ago
|
Iteration: 34.3 → 35.1
Comment 31•10 years ago
|
||
That looks a lot better! Especially on OS X the difference is quite noticible. Would it be possible to also make the button move first when *exiting* customization mode?
Flags: needinfo?(philipp)
Assignee | ||
Updated•10 years ago
|
Attachment #8481605 -
Flags: review?(mconley)
Reporter | ||
Comment 32•10 years ago
|
||
Comment on attachment 8481605 [details] [diff] [review] Synchronously wrap an area (area_tabstrip) before the transition and use requestAnimationFrame instead of setTimeout in doTransition Review of attachment 8481605 [details] [diff] [review]: ----------------------------------------------------------------- Just two nits, but the rest of this looks fine. When you've addressed these two, if you wouldn't mind pinging me so I can give it a final once over, I'd appreciate it. Thanks! ::: browser/components/customizableui/CustomizeMode.jsm @@ +51,5 @@ > } > this.window = aWindow; > this.document = aWindow.document; > this.browser = aWindow.gBrowser; > + this.areas = new Map(); It doesn't look like we ever get the items in this.areas that are keyed off of targets (the area identifier strings). Is there an advantage I'm not seeing? If not, this could / should probably be a Set instead. @@ +971,5 @@ > } > return toolbarItem; > }, > > + _wrapToolbarItem: function* (aArea) { Let's be consistent with the rest of this file, and not put spaces before the function parameter list open parens. (Same with the other functions you introduce)
Attachment #8481605 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #32) > Comment on attachment 8481605 [details] [diff] [review] > Synchronously wrap an area (area_tabstrip) before the transition and use > requestAnimationFrame instead of setTimeout in doTransition > > Review of attachment 8481605 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just two nits, but the rest of this looks fine. When you've addressed these > two, if you wouldn't mind pinging me so I can give it a final once over, I'd > appreciate it. Thanks! > > ::: browser/components/customizableui/CustomizeMode.jsm > @@ +51,5 @@ > > } > > this.window = aWindow; > > this.document = aWindow.document; > > this.browser = aWindow.gBrowser; > > + this.areas = new Map(); > > It doesn't look like we ever get the items in this.areas that are keyed off > of targets (the area identifier strings). Is there an advantage I'm not > seeing? If not, this could / should probably be a Set instead. > Indeed, Sets are implemented as hash tables so using a Set should be just fine.
Assignee | ||
Comment 34•10 years ago
|
||
Use Set instead of Map
Attachment #8481605 -
Attachment is obsolete: true
Attachment #8483807 -
Flags: review?(mconley)
Assignee | ||
Comment 35•10 years ago
|
||
Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=49ee2906de44
Reporter | ||
Comment 36•10 years ago
|
||
Comment on attachment 8483807 [details] [diff] [review] Synchronously wrap an area (area_tabstrip) before the transition and use requestAnimationFrame instead of setTimeout in doTransition Review of attachment 8483807 [details] [diff] [review]: ----------------------------------------------------------------- Let's pull the trigger. Thanks Alex!
Attachment #8483807 -
Flags: review?(mconley) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 37•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ca57c19e84c5
Keywords: checkin-needed
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ca57c19e84c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 35
Comment 39•10 years ago
|
||
This was excellent work Alex - good job.
Assignee | ||
Comment 40•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #39) > This was excellent work Alex - good job. Thank you :-)
You need to log in
before you can comment on or make changes to this bug.
Description
•