1418757 - "New tab" button won't snap to left after customization of the tab row

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[emre.tkg]

Attachment: Before & After customization of tabs row

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

The bug can be reproduced as follows:
1. Open Firefox 57
2. Right-click on top panel (or click on the "open menu" button) and click Customize
3. Add an item to or remove an item from the Tabs row (i.e. the row in the panel where all tabs are), while leaving the New Tab button on the left edge of the items group there.
4. Click "done" to exit customization.


Actual results:

The New Tab button snaps to the rightmost side of the tabs row.


Expected results:

The New Tab button should have snapped to the left (next to the rightmost tab).

Note: The button snaps to the left on a new instance of Firefox or after a restart of Firefox.

Comment 1

[:Gijs (he/him)]

I haven't checked, but I fully expect this is a regression of bug 1370791.

Comment 2

[:Gijs (he/him)]

Given that this is only temporary until a restart, and customizing presumably doesn't happen every run, I think we can leave this for 57 (not worth a dot-release) but we should try to fix for 58.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1418757 - new tab button isn't adjacent to the last tab when customizing in customize mode,

Review commit: https://reviewboard.mozilla.org/r/202016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202016/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8930901 [details]
Bug 1418757 - new tab button isn't adjacent to the last tab when customizing in customize mode,

https://reviewboard.mozilla.org/r/202016/#review207580

::: browser/components/customizableui/test/browser_newtab_button_customizemode.js:20
(Diff revision 1)
> +  if (which == "global") {
> +    isnot(kGlobalNewTabButton.getBoundingClientRect().width, 0,
> +      "main new tab button should be visible");
> +    is(kInnerNewTabButton.getBoundingClientRect().width, 0,
> +      "inner new tab button should be hidden");
> +  } else {

} else if (which == "inner") {
  ...
} else {
  ok(false, "unexpected button: " + which);
}

Comment 6

[:Gijs (he/him)]

Comment on attachment 8930901 [details]
Bug 1418757 - new tab button isn't adjacent to the last tab when customizing in customize mode,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202016/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1490a930ccea
new tab button isn't adjacent to the last tab when customizing in customize mode, r=jaws

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1490a930ccea

Comment 9

[:Gijs (he/him)]

Comment on attachment 8930901 [details]
Bug 1418757 - new tab button isn't adjacent to the last tab when customizing in customize mode,

Approval Request Comment
[Feature/Bug causing the regression]: probably bug 1370791
[User impact if declined]: new tab button is in an odd place after tabstrip customization, until a restart (temporary 'glitch', if you will)
[Is this code covered by automated tests?]: it is now (part of this change)
[Has the fix been verified in Nightly?]: not yet, but has automated tests
[Needs manual test from QE? If yes, steps to reproduce]: it has automated tests, but if QE would like to verify manually, steps are in comment 0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: fundamentally, bug 1370791 improved things so 1 new tab button (either the one next to tabs or the one at the edge of the tabstrip) is always shown. The pre-patch state is that in some cases, the button doesn't show up in the right place. The patch only touches the code that decides "which button" to show, so it seems difficult to come up with a situation where the result would be worse than the current situation. Additionally, it's a very small JS-only patch, and I added an automated test for the code in question.
[String changes made/needed]: no

Comment 10

[:Gijs (he/him)]

Comment on attachment 8930901 [details]
Bug 1418757 - new tab button isn't adjacent to the last tab when customizing in customize mode,

Clearing review to sort out bug 1420583

Comment 11

[:Gijs (he/him)]

Attachment: Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: bug 1370791
[User impact if declined]: new tab button ends up in the wrong place after customization until a restart
[Is this code covered by automated tests?]: they are part of this patch
[Has the fix been verified in Nightly?]: in bug 1420583, yes. Not here directly.
[Needs manual test from QE? If yes, steps to reproduce]: see comment #0
[List of other uplifts needed for the feature/fix]: bug 1420583 fixes a minor oversight in this patch and is included in this exported patchset
[Is the change risky?]: no
[Why is the change risky/not risky?]: small patch, adds additional automated testing, still 6-7 weeks of beta runway. As noted before, the pre-patch state is that in some cases, the button doesn't show up in the right place. The patch only touches the code that decides "which button" to show, so it seems difficult to come up with a situation where the result would be worse than the current situation.
[String changes made/needed]: no.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8932812 [details] [diff] [review]
Patch for beta

Fix a UI regression that the New Tab button snaps to the rightmost. Beta58+.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bedbe646132