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)

x86
All
defect
Not set
normal
Points:
8

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.1

People

(Reporter: mconley, Assigned: alexbardas)

References

Details

(Whiteboard: [Australis:P4])

Attachments

(1 file, 8 obsolete files)

13.10 KB, patch
mconley
: review+
Details | Diff | Splinter Review
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.
Sounds like toolbar items should just be wrapped before the transition rather than after it.
Whiteboard: [Australis:P2] → [Australis:P3]
Attached patch WIP Patch 1 (obsolete) — Splinter Review
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
Ooof, this just seems to hurt us everywhere. I'm guessing that this is because of the wrapping we're doing. Hrm.
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]
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.
Step (C):  __After you close Customization__ the "Make New Tab" icon jumps ... Also, the "List All Tabs" Icon disappears.
To justify the backlog+, this is currently the biggest source of jumpiness in the customization transition.
Flags: firefox-backlog+
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
Whiteboard: [Australis:P4] [ux] p=0 → [Australis:P4] [ux] p=0 [qa-]
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-]
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)
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)
Thanks Madhava, stays on priority list as an ENG bug.
QA Whiteboard: [qa-]
Whiteboard: [Australis:P4] p=0 [qa-] → [Australis:P4]
Iteration: --- → 34.1
Assignee: mconley → nobody
Iteration: 34.1 → ---
Status: ASSIGNED → NEW
Points: --- → 8
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
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)
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)
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)
Attachment #8473984 - Attachment is obsolete: true
Attachment #8473984 - Flags: review?(mconley)
Attachment #8474038 - Flags: review?(mconley)
Iteration: --- → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Hey Alex, I haven't forgotten about this - just had a lot on my plate recently. I'll have a review for you today.
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.
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)
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)
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)
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.
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 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.
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)
Iteration: 34.3 → 35.1
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)
Attachment #8481605 - Flags: review?(mconley)
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+
(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.
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+
Keywords: checkin-needed
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
This was excellent work Alex - good job.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #39)
> This was excellent work Alex - good job.

Thank you :-)
Depends on: 1089591
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: