Closed Bug 1096542 Opened 10 years ago Closed 10 years ago

Tab bar jitters when full

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Version:
35 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 36
Tracking Flags:
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: mmcdonough, Assigned: bgrins)

References

Details

(Whiteboard: [devedition-polish])

Attachments

(3 files, 1 obsolete file)

attempt to repro
(deleted), image/gif
Details
jittering.mp4
(deleted), video/mp4
Details
jitter.patch
(deleted), patch
MattN
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141110004002

Steps to reproduce:

Open enough tabs to fill the tab bar.
Open a new tab with the middle click.


Actual results:

The arrow to scroll through tabs very quickly appears and disappears.


Expected results:

The arrow to scroll through tabs should just stay there.
Is this while using the devedition theme?
Flags: needinfo?(foolkingcrown)
Yes.
Flags: needinfo?(foolkingcrown)
I'm not seeing this on 10.8.5.  Just to make sure, you are not seeing the problem with the default theme (with browser.devedition.theme.enabled = false)?
Attached image tab-not-seeing-jitter.gif (obsolete) (deleted) — 
I'm not seeing the jitter here (osx 10.10), unless if I'm missing something
Attached image attempt to repro (deleted) — 
I tried to repro, not sure what I should be seeing. I don't see jitter.

Are you using a retina display?
Flags: needinfo?(foolkingcrown)
Attachment #8520277 - Attachment is obsolete: true
Here is what it looks like:

https://drive.google.com/file/d/0B-hkR9nzCDDKNm5wQU9DQjZqb2M/view

I noticed that mousing over the final tab stops the jitter.
If I have enough tabs open, it stops. I assume this is the point at which the tabs would stop shrinking in width and the scroll would be enabled.

I am using a Retina display.
This does not happen with the standard theme.
Flags: needinfo?(foolkingcrown)
OK, I can see this too when I customize and add a toolbarbutton to the left of the 'add tab' button.  Not exact same STR, but if I open a bunch of tabs I saw the jitter then closed them right until the scroll arrows got disabled and started seeing it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [devedition-polish]
I believe this is caused by the overriding of tabCurveHalfWidth and tabWidth from Bug 1094782.  That was a fix for the scroll arrows remaining enabled even when they shouldn't.  Removing the whole block (including the .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox that was there before that patch) solves both problems, but adds an extra padding to the sides of the tabs.

It's going to take some more investigation.  I'm guessing there is something else using the magic number 30px or 15px that isn't obvious, or I haven't overridden everything.
(In reply to Andrew Martin McDonough (:mmcdonough) from comment #6)
> Here is what it looks like:
> 
> https://drive.google.com/file/d/0B-hkR9nzCDDKNm5wQU9DQjZqb2M/view

WEEIIIIRRRDDD. And, thanks so much for providing that clip - it really helps. :)
(In reply to Brian Grinstead [:bgrins] from comment #9)
> I believe this is caused by the overriding of tabCurveHalfWidth and tabWidth
> from Bug 1094782.  That was a fix for the scroll arrows remaining enabled
> even when they shouldn't.  Removing the whole block (including the
> .tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox that was there before
> that patch) solves both problems, but adds an extra padding to the sides of
> the tabs.
> 
> It's going to take some more investigation.  I'm guessing there is something
> else using the magic number 30px or 15px that isn't obvious, or I haven't
> overridden everything.

So, first off, that may be the case...

Second, as I noted on IRC at some point, we change the expected tab widths (these are prefs) for Australis. It's possible they may need adjusting for the devtools theme, too, although I'm not entirely sure why, as the total width should be the same - just no more overlap... :-\

Matt, can you help here?
Flags: needinfo?(MattN+bmo)
Can you still reproduce this jitter loop when the all-tabs dropdown isn't on the right of the Tabs Toolbar? That button is no longer customizable since Australis so you would have to restore defaults or edit the pref browser.uiCustomization.state by hand and restart.

Brian, when you reproduced this, was it just that you saw "The arrow to scroll through tabs very quickly appears and disappears." or did you see it get caught in a loop like the video shows?

A quick review of the deveditions styles seems to show all of the tabCurveHalfWidth and tabWidth being overridden. My other guess would have been tab separators causing the problem since hovering the last tab fixes the problem and the hover style differ depending on hover.
Flags: needinfo?(MattN+bmo) → needinfo?(foolkingcrown)
I only ever had the all-tabs dropdown on the left. I tried it on both sides, it happens either way for me.
I'm not sure what you mean by it not being customizable, I can drag it around just fine in custromize-mode.
Flags: needinfo?(foolkingcrown)
(In reply to Andrew Martin McDonough (:mmcdonough) from comment #13)
> I only ever had the all-tabs dropdown on the left. I tried it on both sides,
> it happens either way for me.
> I'm not sure what you mean by it not being customizable, I can drag it
> around just fine in custromize-mode.

You're using an add-on that lets you do that, presumably? For me, it's not movable on latest nightly on OS X - and it shouldn't be...
Ah, yes. I had Classic Theme Restorer installed.

Disabling it makes no difference, though.
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #12)
> Can you still reproduce this jitter loop when the all-tabs dropdown isn't on
> the right of the Tabs Toolbar? That button is no longer customizable since
> Australis so you would have to restore defaults or edit the pref
> browser.uiCustomization.state by hand and restart.

I reproduced a similar jitter loop with devedition on empty profile in a few clicks. See bug 1096655 for steps to reproduce and attachment 8520281 [details] animation there.
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #12)
> Can you still reproduce this jitter loop when the all-tabs dropdown isn't on
> the right of the Tabs Toolbar? That button is no longer customizable since
> Australis so you would have to restore defaults or edit the pref
> browser.uiCustomization.state by hand and restart.
> 
> Brian, when you reproduced this, was it just that you saw "The arrow to
> scroll through tabs very quickly appears and disappears." or did you see it
> get caught in a loop like the video shows?
> 

I reproduced with the all tabs button in the default position (although I did have a button in the tab bar next to the new tab button.  But the reproduction in bug 1096655 doesn't have that special configuration.  I did something like this to reproduce:

Resize window to 975 px
Open 3 tabs
Open a 4th tab using middle click on a link from one of the others
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #12)
> A quick review of the deveditions styles seems to show all of the
> tabCurveHalfWidth and tabWidth being overridden. My other guess would have
> been tab separators causing the problem since hovering the last tab fixes
> the problem and the hover style differ depending on hover.

I don't think that we are changing the box model for the separators, what selector are you talking about?

The only other thing I see that looks suspicious is that we are setting the width/margin of #new-tab-button/.tabs-newtab-button (which I can't test at the moment in the middle of a build).

Where is the JS that is causing the tab resizing to happen?  I could try adding in some logging to try and track it down.
Flags: needinfo?(MattN+bmo)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> (In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #12)
> > A quick review of the deveditions styles seems to show all of the
> > tabCurveHalfWidth and tabWidth being overridden. My other guess would have
> > been tab separators causing the problem since hovering the last tab fixes
> > the problem and the hover style differ depending on hover.
> 
> I don't think that we are changing the box model for the separators, what
> selector are you talking about?

I also didn't see the size of the separator changing for devedition but it's something I would guess without looking at the source.

> The only other thing I see that looks suspicious is that we are setting the
> width/margin of #new-tab-button/.tabs-newtab-button (which I can't test at
> the moment in the middle of a build).

That could be relevant.

> Where is the JS that is causing the tab resizing to happen?  I could try
> adding in some logging to try and track it down.

The tab resizing is just XUL flex IIRC. Overflow stuff is handle by tabbrowser.xml + scrollbox.xml + the place which fires the "overflow" event which I can't find.
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #19)
> > The only other thing I see that looks suspicious is that we are setting the
> > width/margin of #new-tab-button/.tabs-newtab-button (which I can't test at
> > the moment in the middle of a build).
> 
> That could be relevant.

No dice.  Same problem even with the changes to the new tab button removed.
Some more debugging:

* Still a problem with browser.tabs.animate set to false
* Doesn't happen with ctrl+t but does when middle clicking a link on the page
Attached video jittering.mp4 (deleted) — 
Hovering over the last tab stops the jittering
I'm tempted to just remove the entire override since it is a much less bad problem - it just means there are 15 extra px to the left and right of the tab strip between the arrow scrollbars when overflowing
Commenting out the call to tabs.setAttribute("overflow", "true") in the overflow handler here gets rid of the problem: http://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#3573.  Looks like somehow the overflow handler is triggering an overflow event.

I think it's a due to _handleTabSelect calling this line:

this.mTabstrip.ensureElementIsVisible(this.selectedItem, aSmoothScroll);
OK, this is weird - if I change the overridden width to 1px / 2px the jitter still happens.  If I change that to 2px / 4px it isn't a problem (on osx).

I see the exact same problem in Australis (no devedition theme changes) if I change those overrides in tabs.inc.css to:

%define tabCurveWidth 2px
%define tabCurveHalfWidth 1px
Attached patch jitter.patch (deleted) — — Splinter Review 
This fixes the problem (although it's using a magic number that happens to work for me).  Given that this also happens with Australis when using these tab curve values, I'd like to just have a simple fix like this for an immediate and uplifted fix, and track down the underlying issue separately.  I still need to check other platforms - additionally, could anyone who can reproduce this please confirm that the patch fixes it for them?

I've got a try push (which will have binaries when it finishes building) here:  https://tbpl.mozilla.org/?tree=Try&rev=00f6ab844b64
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8520837 - Flags: review?(MattN+bmo)
Comment on attachment 8520837 [details] [diff] [review]
jitter.patch

Review of attachment 8520837 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine for the short term. I wouldn't be surprised if there is a truthiness check that is failing with the 0 somewhere.
Attachment #8520837 - Flags: review?(MattN+bmo) → review+
Maybe this is it here: http://hg.mozilla.org/mozilla-central/annotate/c60fc2c11c0e/browser/base/content/tabbrowser.xml#l3534

It seems like it wasn't made to handle a change from `n < 0` to `n >= 0` at runtime but the devedition stylesheet can get applied after the browser is running. This may not be the actual cause though.
The 'download states' error in the try push (https://tbpl.mozilla.org/?tree=Try&rev=00f6ab844b64) is unrelated - I get it on my local build even without the patch applied
Keywords: checkin-needed
 https://hg.mozilla.org/integration/fx-team/rev/d93e4bc5d08b
Keywords: checkin-needed
Whiteboard: [devedition-polish] → [fixed-in-fx-team][devedition-polish]
I haven't been able to confirm on linux / windows with or without the patch.  Sergey, Andrew, would you be able to check and see if this fixes it for you?

To test this, you can download and run the relevant binaries from https://tbpl.mozilla.org/?tree=Try&rev=00f6ab844b64.  Click the relevant green "B" next to your platform then click "go to build directory"
Flags: needinfo?(sergemp)
Flags: needinfo?(foolkingcrown)
https://hg.mozilla.org/mozilla-central/rev/d93e4bc5d08b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][devedition-polish] → [devedition-polish]
Target Milestone: --- → Firefox 36
Comment on attachment 8520837 [details] [diff] [review]
jitter.patch

Approval Request Comment
[Feature/regressing bug #]: Devedition theme
[User impact if declined]: In certain cases with tab overflowing, the tabs start shaking out of control (see video), it also causes a CPU spike during this time.
[Describe test coverage new/current, TBPL]: None, CSS only
[Risks and why]: Low, CSS only for devedition theme
[String/UUID change made/needed]:
Attachment #8520837 - Flags: approval-mozilla-aurora?
(In reply to Brian Grinstead [:bgrins] from comment #31)
> I haven't been able to confirm on linux / windows with or without the patch.
> Sergey, Andrew, would you be able to check and see if this fixes it for you?
> 
> To test this, you can download and run the relevant binaries from
> https://tbpl.mozilla.org/?tree=Try&rev=00f6ab844b64.  Click the relevant
> green "B" next to your platform then click "go to build directory"

Can't check the bug with this build. It's branded "Firefox Nightly" and has no "Use Firefox Developer Edition Theme" button in about:customizing. I was only able to reproduce it with devedition theme active (see "Probably important things" in bug 1096655) which I can't turn on in this build.
Flags: needinfo?(sergemp) → needinfo?(bgrinstead)
Sergey, you can change the pref browser.devedition.theme.enabled to true in that build and restart.
Flags: needinfo?(bgrinstead) → needinfo?(sergemp)
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #35)
> Sergey, you can change the pref browser.devedition.theme.enabled to true in
> that build and restart.

With browser.devedition.theme.enabled set to true I still see a light theme, not a dark one as in devedition. The button is called "Use Nightly Theme" (browser.devedition.theme.showCustomizeButton=true). The bug does not happen with that "Nightly Theme".

To make sure I also downloaded https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-10-03-02-04-mozilla-central/firefox-36.0a1.en-US.linux-i686.tar.bz2 and set browser.devedition.theme.enabled and browser.devedition.theme.showCustomizeButton to true. And the bug does not happen there either.

I guess it does not happen on Nightly, just on aurora/devedition theme.
Flags: needinfo?(sergemp)
Then you also need to switch to the dark devtools theme through the UI or set the pref devtools.theme to "dark"
 > With browser.devedition.theme.enabled set to true I still see a light theme,
> not a dark one as in devedition. The button is called "Use Nightly Theme"
> (browser.devedition.theme.showCustomizeButton=true). The bug does not happen
> with that "Nightly Theme".

You are doing the right thing, but the strings there are confusing - sorry!  Basically, the button is just hardcoded to "Use [product name] theme" in the assumption that it will only show up in dev edition.

If you are not seeing the problem here, then I'd say it's verified to be fixed on your system (the light/dark devedition theme will have no bearing on this particular issue).  Thanks for following up - this should be fixing on devedition once it gets approval and gets uplifted.
(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22) from comment #37)
> Then you also need to switch to the dark devtools theme through the UI or
> set the pref devtools.theme to "dark"

That makes the difference! The bug did not happen with devtools.theme="light" for some Nightly.

(In reply to Brian Grinstead [:bgrins] from comment #38)
> If you are not seeing the problem here, then I'd say it's verified to be
> fixed on your system (the light/dark devedition theme will have no bearing
> on this particular issue).  Thanks for following up - this should be fixing
> on devedition once it gets approval and gets uplifted.

I guess so. Tested https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ :
2014-11-10-03-02-04-mozilla-central - Nightly 2014-11-10 - bug 1096655 for "dark", but not "light"
2014-11-11-03-02-03-mozilla-central - Nightly 2014-11-11 - bug 1096655 for both "dark" and "light"
2014-11-12-03-02-02-mozilla-central - Nightly 2014-11-12 - no bug for both "dark" and "light"

about:config changes used for test:
  browser.devedition.theme.enabled = true
  browser.devedition.theme.showCustomizeButton = true
  devtools.theme = dark

Thank you!
Attachment #8520837 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1100578
Depends on: 1111091
Bugday-20141218: The arrow to scroll behaves as expected.
Flags: needinfo?(pratyasmitamishra)
Component: Untriaged → Theme
Flags: needinfo?(pratyasmitamishra)
Flags: needinfo?(foolkingcrown)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: