Closed Bug 1066531 Opened 10 years ago Closed 9 years ago

[e10s] Switching tabs can result in old content being displayed for a split second after the tab bar is updated

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 39
Iteration:
39.3 - 30 Mar
Tracking Flags:
Tracking Status
e10s m5+ ---
firefox39 --- fixed

People

(Reporter: ttaubert, Assigned: gw280)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(5 files, 8 obsolete files)

0001-Bug-1066531-Delay-tab-switching-until-content-is-rea.patch
46.77 KB, patch
mconley
: review+
mstange
: review+
Details | Diff | Splinter Review
MozReview Request: Bug 1066531 - Delay tab switching until content is ready in e10s mode r=mconley,mstange
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1132072 - Handle RequestNotifyLayerTreeReady when RenderFrameParent not ready (r=dvander)
39 bytes, text/x-review-board-request
Details
MozReview Request: commit 29a4666ef6d80a55333a1d7d489db3f663fc4478
39 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1132072 - billm's stuff.
39 bytes, text/x-review-board-request
Details 
Switching between tabs in e10s doesn't feel as smooth as when using a non-e10s window. For a split second the old content stays visible as it seems to take a while for the newly selected tab's content to show up.

This doesn't seem like a blocker for testing but something we probably wouldn't want to ship to users.
This is probably related to bug 1009628.
Blocks: 1009628
Component: General → Tabbed Browser
Blocks: e10s
In bug 986782 comment #8 Benoit said he dislikes the way Chrome did it. I actually really like that idea. Sure there might be cases where content changes but that doesn't sound like the default case and having some of the page change a few ms after switching tabs is probably better than nothing happening for a few ms. Can we talk about this again?
Gavin, why did you mark this as m3+?
Flags: needinfo?(gavin.sharp)
Probably because the milestone definitions keep changing and I'm out of date as to which means what. Feel free to adjust as you see fit.
Flags: needinfo?(gavin.sharp)
With bug 1008435 fixed, we should get some profiles.
Can we make this critical?  I don't see how we can ship without fixing this, and furthermore it is harming the productivity of everyone using nightly, which includes the majority of our staff I would assume.

I have seen many people complain about this during the work week, but because they were working on their project they didn't have time to report this, due to the lost time it causes.
Flags: needinfo?(gavin.sharp)
I agree that this is an annoying problem. I don't think it's a frontend issue in general, though (except perhaps if the frontend is using CPOWs too much). Dave, do you know what the plans are to address this?
Flags: needinfo?(gavin.sharp) → needinfo?(dtownsend+bugmail)
I don't think we totally understand why the content process is taking so long to respond. Getting profiles when it happens would help but I'll put it back on the triage list for discussion on Thursday.
tracking-e10s: + → ---
Flags: needinfo?(dtownsend+bugmail)
George is going to investigate with gfx team.
tracking-e10s: --- → ?
Flags: needinfo?(gwright)
test case:

1) open up three or four tabs with heavy weight pages, treeherder with 50 checkins compressed works well.
2) intersperse about:newtabs between each treeherder tab
3) switch between tabs, between a about:newtab and a treeherder tab works well

result: you should see a delay in tab painting, with a white content window occasionally bleeding through.

To increase the side effects, load additional entries into each treeherder tab.
So is the problem with interactions between remote and non-remote tabs?
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #13)
> So is the problem with interactions between remote and non-remote tabs?

couple problems afaict:

1) remote tabs sometimes paint a blank content area before refreshing if the content process is dragged down. Is the compositor waiting for content layer updates and not receiving them in time?

2) if we're supposed to show some sort of placeholder here (spinner or stale tab content), it doesn't work.
The spinner usually works fine for me (except after content process crashes, in some cases). It just shows up way too often.
tracking-e10s: ? → +
Should not it be set to "CRITICAL"?

The giant spinner/flower (or whatever this thing is called), endless "Connecting..." tab label changes on and off, bug #1115683 (https://bugzilla.mozilla.org/show_bug.cgi?id=1115683) about excessive TabAttrModified event fires -- all of it combined has made things for me unbearable, I had to turn e10s off because besides testing I have some work to do. I can not take this any more; e10s works good only in tiny sessions. It is fine for by far most of users, but not for "prosumers".
(In reply to User Dderss from comment #16)
> Should not it be set to "CRITICAL"?

Sorry to hear that but we're working on getting e10s to a state where it's usable for power users like you as well. That fact doesn't merit setting this bug to critical though, most of our Nightly population can handle the minor issues - probably because their sessions are smaller? Before shipping e10s we surely will have to look at bigger sessions and figure out what's going wrong here...
(In reply to User Dderss from comment #18)
> Is there "lazy load" project for SessionStore? It would help greatly,
> including with Electrolysis, including with this slow switching tabs issue.
> FireFox should be lightning fast to load and operate no matter how big your
> session is, if "click-to-load" option is turned on.
> 
> If there is still no one assigned to this, could you please consult with
> colleagues to start such project? Thank you in advance.

Please take this discussion to either another bug report or the newsgroups. It isn't relevant to the problem being worked on here.
Clearing the needinfo so we can discuss this in triage tomorrow
Flags: needinfo?(gwright)
Assignee: nobody → gwright
tracking-e10s: ? → m5+
Here's a profile that might help with the investigation (it was too large to upload through cleopatra, hopefully this format will do):
http://people.mozilla.org/~mleibovic/tmp/tab-switch-profile.dms

mconley also asked me to capture what's in about:compartments, so here you go:

time 	time in CPOWs 	name
0μs	83933000μs	<non-addon>
159862536μs	46356000μs	Bugzilla Tweaks
23088μs	0μs	JavaScript Terminal
56436μs	0μs	fx-devtools
1423μs	0μs	about:me
401803μs	0μs	Bugzilla Secure Mail Viewer for GMail
19731μs	0μs	Clean Links
542325582μs	172195000μs	Pin It Button
127488318μs	10118000μs	Firefox Interest Dashboard
45670μs	0μs	ADB Helper
40692810μs	5787000μs	Gecko Profiler
31040579μs	4862000μs	Mozilla Tree Status
279523μs	0μs	SQLite Manager
2891μs	0μs	Mass Password Reset
For those watching at home, margaret's profile is too large to upload to the Cleopatra server, but the link she's posted can be inserted into the text box at the bottom of this page: http://people.mozilla.org/~bgirard/cleopatra/

After it downloads, it will display.

It looks to me as if there's a _ton_ of traffic going over the nsIObserver add-on shim. So, I immediately suspect an add-on. The Pin It Button add-on appears to be using the most CPOW time. If you disable that, do you notice an improvement?
Flags: needinfo?(margaret.leibovic)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #24)
> For those watching at home, margaret's profile is too large to upload to the
> Cleopatra server, but the link she's posted can be inserted into the text
> box at the bottom of this page: http://people.mozilla.org/~bgirard/cleopatra/
> 
> After it downloads, it will display.
> 
> It looks to me as if there's a _ton_ of traffic going over the nsIObserver
> add-on shim. So, I immediately suspect an add-on. The Pin It Button add-on
> appears to be using the most CPOW time. If you disable that, do you notice
> an improvement?

This suspect appears guilty. Disabling the add-on significantly improves things for me. I filed bug 1130629.
Flags: needinfo?(margaret.leibovic)
I hit some sluggish tabs today. Here is a profile:

http://people.mozilla.org/~bgirard/cleopatra/#report=aefbf1f761cf61be0e94d135cee4c1cb6e11cec2&selection=0,1,2,170,6,171,172,6,173,6,174,6,58,6,59

My about:compartments:

time 	time in CPOWs 	name
0μs	156591000μs	<non-addon>
20197μs	0μs	Restartless Restart
13371569μs	1578000μs	tabsmack
20555μs	0μs	Firefox OS 2.1 Simulator
27588μs	0μs	fx-devtools
379300638μs	72333000μs	Gecko Profiler
323939499μs	64209000μs	Mozilla Tree Status
27970μs	0μs	Saved Password Editor
16542μs	0μs	DOM Inspector
13134μs	0μs	SQLite Manager
108854920μs	57855000μs	skiptco
Another profile to look at - I had some particularly sluggish tabs, and then I switched to a tab that contained a rather large test run log. I saw the spinner for about 2-3 seconds before it resolved itself.

Here's what the profile captured:

http://people.mozilla.org/~bgirard/cleopatra/#report=3f703670e9a2b438e692dc2070c708edf7409e6a&selection=0,880,71,34,1145

The content process seems to do two large, slow reflows, one right after the other.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27)
> Another profile to look at - I had some particularly sluggish tabs, and then
> I switched to a tab that contained a rather large test run log. I saw the
> spinner for about 2-3 seconds before it resolved itself.
> 
> Here's what the profile captured:
> 
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=3f703670e9a2b438e692dc2070c708edf7409e6a&selection=0,880,71,34,1145
> 
> The content process seems to do two large, slow reflows, one right after the
> other.

As this bug was originally reported about the timing disconnect between the act of switching tabs, and the tab content being displayed, I've filed bug 1135719 to keep track of tabs sometimes taking a long time to paint issue.
So I tried a bunch of approaches here, and I think this is the least invasive. Basically, what I'm doing here is I'm changing the meaning of tab._selected to mean "we're selected, but not visually, so do your thing", and then we only actually visually select the tab when we decide to finalise the tab switch (ie - after we get mozAfterRemotePaint).

This has the advantage of us reusing all the machinery that's currently in place to deal with tab switching so that I don't have to deal with all the corner cases currently accounted for like users switching tabs again before the tab is fully switched, etc.
Attachment #8569499 - Flags: review?(mconley)
(In reply to George Wright (:gw280) from comment #29)
> So I tried a bunch of approaches here, and I think this is the least
> invasive. Basically, what I'm doing here is I'm changing the meaning of
> tab._selected to mean "we're selected, but not visually, so do your thing",
> and then we only actually visually select the tab when we decide to finalise
> the tab switch (ie - after we get mozAfterRemotePaint).

Does this mean that there will be a noticeable lag when switching tabs?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30)
> (In reply to George Wright (:gw280) from comment #29)
> > So I tried a bunch of approaches here, and I think this is the least
> > invasive. Basically, what I'm doing here is I'm changing the meaning of
> > tab._selected to mean "we're selected, but not visually, so do your thing",
> > and then we only actually visually select the tab when we decide to finalise
> > the tab switch (ie - after we get mozAfterRemotePaint).
> 
> Does this mean that there will be a noticeable lag when switching tabs?

No, if we hit a timeout (300ms) we will switch the tabs anyway and show a spinner.
Comment on attachment 8569499 [details] [diff] [review]
0001-tabs-switching-after-remote-paint.patch

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

Thanks George.

So we're clear, the summary of this bug is quite poor. What we're actually fixing here is the disconnect between the tab strip and the tabpanels.

Context:

In bug 1009628, I made it so that the tabstrip would shift immediately and not be blocked on waiting for the content to paint. The tabpanels would then update once content was ready to paint, and would show a spinner after a 300ms timeout.

This causes the disconnect that George is attempting to fix; the idea here is that the tab strip and the tabpanels should switch in lock-step with one another, and that we should wait for the content to be ready to paint before visually switching both the selected tab and the tabpanels. If we hit a 300ms threshold, we'll switch the selected tab and tabpanel, but show a spinner.

A few issues:

1) This patch appears to break non-e10s windows - the selected tab never changes.
2) The URL bar and all of the nav-bar icons are updated before the selected tab and tabpanels switch (due to the updateCurrentBrowser call firing before we visually switch the tabs/panels). This was true before, but I thought it was worth pointing out.

I'm curious to know what other people familiar with tabbrowser think about this, and potential solutions. I'm loathe to do too much refactoring inside tabbrowser.xml, since it's all very critical to the functioning of the browser.

jaws, I'm going to pick on you; can you think of better solutions for what we're trying to do here?
Attachment #8569499 - Flags: review?(mconley) → review-
Flags: needinfo?(jaws)
Attached patch tabs-again.patch (obsolete) — — Splinter Review 
Minor update to fix the non-e10s case. mconley and I are still discussing what the best thing to do re. the URL bar is.
Attachment #8569499 - Attachment is obsolete: true
Summary: [e10s] Switching tabs feels sluggish → [e10s] Switching tabs can result in old content being displayed for a split second after the tab bar is updated
Attached patch tabs-take-3.patch (obsolete) — — Splinter Review 
So I think this should cover most of the issues we found. It may introduce more though. Who knows!
Attachment #8570022 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Attachment #8570068 - Flags: review?(mconley)
So I've been mulling this over in my head, and I'm a bit worried - specifically, I'm worried about URL bar focus.

With single-process Firefox, as folks here are probably aware, we block on painting the web content of the newly selected, or newly opened tab.

Let's consider the new tab case. For me, I often open a tab with Cmd-T, and immediately start typing a URL since I know that the URL bar is going to be focused by the time my keyboard events start getting consumed.

This will no longer be the case with this patch, and it worries me; I suspect that people will open or switch to tabs, and the first few characters they start to type are going to get lost because the URL bar won't get focus until the content is ready to paint. Because we're not blocked on waiting for the content to paint, the keyboard events will get serviced before the URL bar is focused.

Something to mull over tomorrow when we start looking at this again.
Comment on attachment 8570068 [details] [diff] [review]
tabs-take-3.patch

Having thought about this, I think the better of the two evils is probably to have the URL bar and toolbar buttons be updated synchronously, even though we're waiting for the tab to paint in order to show selection.

I _think_ that's better.
Attachment #8570068 - Flags: review?(mconley) → review-
Attached patch tabs4.patch (obsolete) — — Splinter Review 
Enjoy.
Attachment #8570068 - Attachment is obsolete: true
Attachment #8570648 - Flags: review?(mconley)
Comment on attachment 8570648 [details] [diff] [review]
tabs4.patch

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

::: browser/base/content/tabbrowser.xml
@@ +5036,5 @@
> +          // in e10s we want to only pseudo-select a tab before its rendering is done, so that
> +          // the rest of the system knows that the tab is selected, but we don't want to update its
> +          // visual status to selected until after we receive confirmation that its content has painted.
> +          if (val)
> +            this.setAttribute("logically-selected", "true");

Out of curiosity, why are you using an attribute to store this value, instead of a field on the binding?

@@ +5041,5 @@
> +          else
> +            this.removeAttribute("logically-selected");
> +
> +          // If we're non-e10s we should update the visual selection as well at the same time
> +          if (!this.parentNode._pendingTab.linkedBrowser.isRemoteBrowser) {

I think we can just check !gMultiProcessBrowser here. And I don't think we need _pendingTab anymore.

@@ +5050,5 @@
> +      </property>
> +
> +      <property name="_visuallySelected">
> +        <setter>
> +          <![CDATA[

So this is copied verbatim from the _selected setter for the tabbox tab binding.

I wonder if it makes more sense to alter the tabbox tab binding to a helper method, which the tabbox _selected setter can call.

Then, our binding here can call it from _visuallySelected. That way we don't have to duplicate all of this stuff.
Attachment #8570648 - Flags: review?(mconley) → review-
Attached patch tabs5.patch (obsolete) — — Splinter Review 

        Attachment #8570648 -
        Attachment is obsolete: true

        Attachment #8570738 -
        Flags: review?(mconley)

    
    

    
  
Comment on attachment 8570738 [details] [diff] [review]
tabs5.patch

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

Let's wait until the try run gets in, but this I've got a tentative r+ with nit in the chamber here.

::: browser/base/content/tabbrowser.xml
@@ +5038,5 @@
> +          // in e10s we want to only pseudo-select a tab before its rendering is done, so that
> +          // the rest of the system knows that the tab is selected, but we don't want to update its
> +          // visual status to selected until after we receive confirmation that its content has painted.
> +          if (val)
> +            this._logicallySelected = true;

this._logicallySelected = val

    
    

    
  
browser/base/content/test/general/browser_bug563588.js appears to hate this patch. :/

    
    

    
  
Comment on attachment 8570738 [details] [diff] [review]
tabs5.patch

r- for the test failure. :/

        Attachment #8570738 -
        Flags: review?(mconley) → review-

    
    

    
  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd97e0f35038

The issue with that test was that when moving tabs, it called _selected to update the visual style, but _selected no longer updates the visual style until after we get mozAfterRemotePaint. This changes that to call _visuallySelected instead.

    
    

    
  
OK, so we've hit a fork in the road here, and I'm not sure which way to go down.

Basically, the intent of this bug is to delay the tabbar from updating the visual tab selection until we have that tab's content ready to display (either the tab's actual content, or a spinner showing the tab's content is delayed for some reason).

The initial road I took was to split out the tab's _selected property to a _logicallySelected, and a _visuallySelected one. _logicallySelected would be set when we request a new tab be selected (e.g. by clicking on a tab, or by calling .selectedTab), and then _visuallySelected would be called when we show the tab's content. _visuallySelected would then set the "selected" attribute on the tab element, and the CSS would kick in to style the tabs properly.

The issue here is that some of our tests fail due to code like:

1: gBrowser.selectedTab = tab1;
2: is(tab1.getAttribute("selected"), true);

This is because line 2 is executed before the tab's content is ready, so the "select" attribute hasn't been applied yet. That is, the test assumes synchronous tab switching.

mconley and I have discussed this and the alternative approach would be to not much about with the _select property, and instead set a new attribute on the tab element like "visually-selected", which would then trigger the CSS changes, instead of "select".

The issue there is that we'd then break all themeing.

Which route do we want to take? I'm inclined to think we should take the first route, and change our tests to no longer assume synchronous tab selection, as that's what the intent of this patch is. mconley thinks that might open a rats nest, but is unsure of the impact of themeing being broken.

MattN: your name came up as someone who might have some insight into this. Got any ideas?
Flags: needinfo?(MattN+bmo)

    
    

    
  
(In reply to George Wright (:gw280) from comment #45)
> 1: gBrowser.selectedTab = tab1;
> 2: is(tab1.getAttribute("selected"), true);
> 
> This is because line 2 is executed before the tab's content is ready, so the
> "select" attribute hasn't been applied yet. That is, the test assumes
> synchronous tab switching.

I suspect many add-ons also make this same assumption and I think it would be hard to fix even all of our browser code to not assume this due to there being so many methods which end up switching tabs. This type of change is likely to cause security bugs due to this assumption being violated as code might get mixed up about which tab is performing an action.

> mconley and I have discussed this and the alternative approach would be to
> not much about with the _select property, and instead set a new attribute on
> the tab element like "visually-selected", which would then trigger the CSS
> changes, instead of "select".
> 
> The issue there is that we'd then break all themeing.

Right, it's common to style the selected tab differently and any add-ons that were doing that would need to be updated. If we replace @selected with @visually-selected then add-ons aren't going to have the selected tab distinguished which would affect users. OTOH, if we just add a new attribute @visually-selected (leaving @selected applying when logically selected) then it's not ideal as extensions changing the selected tab styling will suffer from this bug still but that is better than not having a selected tab identifiable at all. The downside of leaving the old attribute is that it makes it less obvious to add-on developers and users of the add-ons that the new attribute should be used. We can do evangelism, warning for usage of @selected in automated add-on validation, and let add-on reviewers know about this change to help get add-ons fixing this bug.

> Which route do we want to take? I'm inclined to think we should take the
> first route, and change our tests to no longer assume synchronous tab
> selection, as that's what the intent of this patch is. mconley thinks that
> might open a rats nest, but is unsure of the impact of themeing being broken.

Since you can't fix add-ons that make the same assumption and the consequences could involve data loss and/or security issues with approach 1, I would prefer approach 2 while leaving @selected which only involves visual inconsistencies for up to 300ms IIUC.
Flags: needinfo?(MattN+bmo)
Flags: firefox-backlog+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        0001-Delay-tab-switch.patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8570738 -
        Attachment is obsolete: true

        Attachment #8575513 -
        Flags: review?(mconley)

    
    

    
  
Comment on attachment 8575513 [details] [diff] [review]
0001-Delay-tab-switch.patch

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

Hmm… I was hoping this could have been isolated to browser/ but I can see why you changed toolkit too. Changes to xul.css require review from Enn though.

        Attachment #8575513 -
        Flags: review?(enndeakin)

    
    

    
  


  
/r/5191 - Bug 1132072 - Handle RequestNotifyLayerTreeReady when RenderFrameParent not ready (r=dvander)
/r/5193 - Bug 1132072 - Tab switch refactoring (r=mconley?)
/r/5195 - Bug 1066531 - Delay tab switch

Pull down these commits:

hg pull review -r 729710dbeb05645ba3faa810316221061bcf2a26

    
    

    
  
https://reviewboard.mozilla.org/r/5195/#review4199

Should we update this code in toolkit/themes/osx|windows|linux/global/in-content/common.css:

```
xul|tab[selected] {
  text-shadow: none;
}
```

I know it's for in-content prefs, but if we're changing the semantics of tabs so that they can be logically and visually selected... that'll apply to tabs inside in-content prefs as well, and shouldn't we be consistent? Maybe that's a question for Enn when he reviews.

You also need to update the browser-lightweightTheme.css files.

I think you also need to update the sub-elements that used to rely on the selected attribute for styling, example: https://hg.mozilla.org/mozilla-central/file/6686aacf006f/browser/themes/linux/browser.css#l1852

Another example: https://hg.mozilla.org/mozilla-central/file/6686aacf006f/browser/themes/osx/browser.css#l3069

Basically, what I think you need to do is grep the browser/themes directory for "selected" and "tab", and see if any of the hits need to be updated.

::: toolkit/themes/windows/global/tabbox.css
(Diff revision 1)
> -tab[selected="true"] {
> +tab[visually-elected="true"] {

visually-elected -> visually-selected

::: browser/base/content/tabbrowser.xml
(Diff revision 1)
> +            this.selectedTab._visuallySelected = true;

When we close the selected tab, and \_endRemoveTab gets fired when the transition ends, we're switching to some other tab (I think it's usually the last selected tab). Don't we need to wait for that tab to be ready to paint? Shouldn't we not set \_visuallySelected to true until the content for that tab is ready?

::: browser/base/content/tabbrowser.xml
(Diff revision 1)
> +                if (this.lastVisibleTab != this.visibleTab) {
> +                  this.lastVisibleTab._visuallySelected = false;
> +                  this.visibleTab._visuallySelected = true;
> +                }

I think this is causing a glitch. I think this can potentially put a tab in a state where it is \_not\_ logically selected, but \_is\_ visually selected. That's glitching us out pretty hard - when you switch back and forth between tabs, I can see this for a split second; the tab title colour is wrong, and the top of the tab appears to be missing.

You can easily reproduce the visual glitch by setting \_visuallySelected to true on a tab that is not \_logicallySelected.

        Attachment #8575513 -
        Flags: review?(mconley) → review-

    
  
Flags: needinfo?(bosswieslaw77)

    
  
Flags: needinfo?(bosswieslaw77)

    
    

    
  
Comment on attachment 8575513 [details] [diff] [review]
0001-Delay-tab-switch.patch

>             // update tab positional properties and attributes
>             this.selectedTab._selected = true;
>+            this.selectedTab._visuallySelected = true;
>             this.tabContainer._setPositionalAttributes();
> 
>             // Removing the panel requires fixing up selectedPanel immediately
>@@ -2758,6 +2759,7 @@
> 
>           aIndex = aIndex < aTab._tPos ? aIndex: aIndex+1;
>           this.mCurrentTab._selected = false;
>+          this.mCurrentTab._visuallySelected = false;
> 
>           // invalidate caches
>           this._browsers = null;
>@@ -2770,8 +2772,10 @@
>           for (let i = 0; i < this.tabs.length; i++) {
>             this.tabs[i]._tPos = i;
>             this.tabs[i]._selected = false;
>+            this.tabs[i]._visuallySelected = false;
>           }
>           this.mCurrentTab._selected = true;
>+          this.mCurrentTab._visuallySelected = true;

The callers above will try to set _visuallySelected twice when gMultiProcessBrowser is false. Perhaps we should move setting these into a separate method, which would also avoid any chance of error creeping in.


>     <content>
>-      <xul:hbox class="tab-middle box-inherit" xbl:inherits="align,dir,pack,orient,selected" flex="1">
>+      <xul:hbox class="tab-middle box-inherit" xbl:inherits="align,dir,pack,orient,selected,visually-selected" flex="1">

Please no hyphens in xul attribute names (use visuallyselected instead)

 
>+      <property name="_logicallySelected">
>+        <setter>
>+          <![CDATA[
>+          if (val)
>+            this.setAttribute("selected", "true");
>+          else
>+            this.removeAttribute("selected");
>+          ]]>
>+        </setter>
>+      </property>
>+
>+      <property name="_selected">
>+        <setter>
>+          <![CDATA[
>+          // If our tab switching is synchronous, then logical selection = visual selection
>+          this._logicallySelected = val;
>+          this._visuallySelected = val;
>+          ]]>
>+        </setter>
>+      </property>
>+

For completeness, setters should always return val. It's a bit weird to not have a getter for these though, but I guess that's how it was before.


>diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css
>-tab[selected="true"]:not([ignorefocus="true"]) {
>+tab[visually-selected="true"]:not([ignorefocus="true"]) {
>   -moz-user-focus: normal;
> }
> 

This is used to prevent tabs from focusing when you click on them, so you want to use the selected attribute here.


You will also need to update the native theme code to deal with the attribute change as well. (search for nsGkAtoms::selected)

Note also that accessibility tools will see the tab as selected when logicallySelected is set to true and won't detect the change to visuallySelected at all. That might be ok, but wanted to confirm that is what you expected.

    
  

        Attachment #8575513 -
        Flags: review?(enndeakin)

    
    

    
  
Neil, do you have any comment on mconley's question at the beginning of comment 50?
Flags: needinfo?(enndeakin)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        tabs.patch (obsolete)
        — 
          — Splinter Review
      

    


  
So a few comments:

I've changed the semantics in updateDisplay to be more correct. I don't see any visual anomalies on the current system. The nullptr check is necessary to fix the crashes I was seeing on try I think (haven't pushed to try yet, will do that momentarily).

I've renamed the attribute to "visuallyselected" as per Neil's request, and I've also gone through and changed all the instances of "selected" in the CSS under browser/themes to "visuallyselected", when they pertain to tabs. I wasn't sure about the CSS that applies to ".ac-*-text" such as at http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/browser.css#1459 though.

As for setting visuallySelected twice when calling logicallySelected and visuallySelected together, I decided against creating a separate method as its usage seems to be very limited, so I changed the code to call logicallySelected and visuallySelected's setters explicitly.

Regarding the native theming stuff, I did a quick grep in the codebase for "nsGkAtoms::selected" and only found stuff regarding things like checking the selected item on a <select> element, or things that aren't actually this very specific tab element. Can you elaborate further on where you think the impact is for this?

I'll wait for Neil's weigh-in on the in-content stuff before I go ahead and make any changes there.

        Attachment #8575513 -
        Attachment is obsolete: true

        Attachment #8576949 -
        Flags: review?(mconley)

    
    

    
  
(In reply to George Wright (:gw280) from comment #53)
> I wasn't sure about the CSS that applies to ".ac-*-text" such as at
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/browser.css#1459 though.

AFAIK this is unrelated to tabs as it's related to the selected item in the autocomplete dropdown (e.g. on the URL bar).

    
    

    
  
> xul|tab[selected] {
>   text-shadow: none;
> }

Offhand, it looks like something that would need to change.

> Regarding the native theming stuff, I did a quick grep in the codebase for
> "nsGkAtoms::selected" and only found stuff regarding things like checking
> the selected item on a <select> element, or things that aren't actually this
> very specific tab element. Can you elaborate further on where you think the
> impact is for this?
> 

I gave it a quick but not complete look. There's nsNativeTheme.h::IsSelectedTab for one. Also some usage in WidgetStateChanged implementations that check whether the widget should be repainted. It's possible they don't affect the tabbrowser ui but I don't know about that.
Flags: needinfo?(enndeakin)

    
    

    
  


  
Hopefully final iteration of this patch. It's green on try, except for the failures caused by :billm's patch (which he's currently working on).

Flagging Markus for review for the native widget theme stuff.

        Attachment #8576949 -
        Attachment is obsolete: true

        Attachment #8576949 -
        Flags: review?(mconley)

        Attachment #8578158 -
        Flags: review?(mstange)

        Attachment #8578158 -
        Flags: review?(mconley)

        Attachment #8578158 -
        Flags: review?(mstange) → review+

    
    

    
  
Comment on attachment 8576130 [details]
MozReview Request: bz://1066531/mconley

/r/5191 - Bug 1132072 - Handle RequestNotifyLayerTreeReady when RenderFrameParent not ready (r=dvander)
/r/5193 - Bug 1132072 - billm's stuff.
/r/5195 - commit 29a4666ef6d80a55333a1d7d489db3f663fc4478
/r/5729 - Bug 1066531 - Delay tab switching until content is ready in e10s mode r=mconley,mstange

Pull down these commits:

hg pull review -r 7046e698bc356a0be1ef0b44c00cb677e0925ba1

    
    

    
  
https://reviewboard.mozilla.org/r/5729/#review4681

The only glitch I've found is that the tab close button appears in the logically selected case instead of the visually selected case. I'm OK for that to get fixed in a follow-up though, since I know this is a big patch that's been languishing.

Can you please file a bug for that?

I used MattN's [mozscreenshots](https://github.com/mnoorenberghe/mozscreenshots) tool to compare before and after with this patch (using the Tabs, LightweightThemes, PrivateBrowsing, TabsInTitlebar, DevEdition configurations) and didn't see anything else worth mentioning.

::: toolkit/content/widgets/tabbox.xml
(Diff revision 1)
> +            this.removeAttribute("selected");

Setters should return val.

::: toolkit/content/widgets/tabbox.xml
(Diff revision 1)
> +          this._visuallySelected = val;

Same as above - you should return val.

    
    

    
  
Comment on attachment 8578158 [details] [diff] [review]
0001-Bug-1066531-Delay-tab-switching-until-content-is-rea.patch

See comment 59. r=me with the fixes I mention and the follow-up filed for the tab close button glitch.

        Attachment #8578158 -
        Flags: review?(mconley) → review+

    
    

    
  
I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/532dea9ce216 to be able to back out bug 1132072, which caused near-permafailing e10s-bc3.
Flags: needinfo?(gwright)

    
    

    
  
Saw on #developers, but thanks for the heads up!
Flags: needinfo?(gwright)

    
  
Blocks: 1135719

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8194018355f6
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox39:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Iteration: --- → 39.3 - 30 Mar
Flags: qe-verify?

    
    

    
  
Pretty sure this caused intermittent test 1121671 to fail much more regularly.

    
  
Depends on: 1148879
Depends on: 1157404

    
    

    
  

        Attachment #8576130 -
        Attachment is obsolete: true

    
    

    
  
FYI (mainly for addon authors suffered from this change)

This change breaks compatibility of some non-sdk-based addons, like my Tree Style Tabs, Informational Tab, and others. These addons includes custom CSS rules to fix appearance of selected tabs broken with modifications by the addon itself. It is written with selectors like:

  .custom-class .tabbrowser-tab[selected] { ... }

and it cannot be replace with the new "visuallyselected" attribute like:

  .custom-class .tabbrowser-tab[visuallyselected] { ... }"

because it never work on Firefox 40 and older versions. Remember, even if Firefox 41 is released, Firefox 38ESR is still alive.

So, for addon authors like me, I've released a tiny library to backport this "visuallyselected" attribute for Firefox 31-40. This introduces the "visuallyselected" attribute for tabs, and it will help you to migrate your addon from "selected" to "visuallyselected" without any dirty hacks to support both old Firefox and new one.

Repository
https://github.com/piroor/fxaddonlib-visuallyselected-tabs

Usage (migration example)
https://github.com/piroor/treestyletab/commit/523848a7ed2bc69d486ed201957b7d07c6e774cc

    
    

    
  
(In reply to YUKI "Piro" Hiroshi from comment #72)
> FYI (mainly for addon authors suffered from this change)
> 
> This change breaks compatibility of some non-sdk-based addons, like my Tree
> Style Tabs, Informational Tab, and others. These addons includes custom CSS
> rules to fix appearance of selected tabs broken with modifications by the
> addon itself. It is written with selectors like:
> 
>   .custom-class .tabbrowser-tab[selected] { ... }
> 
> and it cannot be replace with the new "visuallyselected" attribute like:
> 
>   .custom-class .tabbrowser-tab[visuallyselected] { ... }"
> 
> because it never work on Firefox 40 and older versions. Remember, even if
> Firefox 41 is released, Firefox 38ESR is still alive.
> 
> So, for addon authors like me, I've released a tiny library to backport this
> "visuallyselected" attribute for Firefox 31-40. This introduces the
> "visuallyselected" attribute for tabs, and it will help you to migrate your
> addon from "selected" to "visuallyselected" without any dirty hacks to
> support both old Firefox and new one.
> 
> Repository
> https://github.com/piroor/fxaddonlib-visuallyselected-tabs
> 
> Usage (migration example)
> https://github.com/piroor/treestyletab/commit/
> 523848a7ed2bc69d486ed201957b7d07c6e774cc

Why not using something like:

.custom-class .tabbrowser-tab:not([visuallyselected])[selected],
.custom-class .tabbrowser-tab[visuallyselected] { ... }

Notice that 3-party themes (also affected by this change) can't import any javascript module

    
    

    
  
(In reply to Pardal Freudenthal (:ShareBird) from comment #73)
> .custom-class .tabbrowser-tab:not([visuallyselected])[selected],

The selector matches to a tab "going to be newly selected" also. You'll see two "visually selected" tabs unexpectedly.

> Notice that 3-party themes (also affected by this change) can't import any javascript module

I think that the lifetime of addons is longer than full-themes' one. Full-themes must be rewrited for each versions of Firefox in general because there is no scriptable feature in CSS. Moreover  full-themes are possibly killed in the future completely. (Of course, old-style addons like mine also may be killed in the future...)
https://groups.google.com/forum/#!msg/firefox-dev/Mtlsjme_h_Y/S7jibiERWPkJ

    
    

    
  
I suggest using the chrome registration system to tie specific versions of the CSS to the appversion:

https://developer.mozilla.org/en-US/docs/Chrome_Registration#appversion

    
    

    
  
(In reply to George Wright (:gw280) from comment #75)
> I suggest using the chrome registration system to tie specific versions of
> the CSS to the appversion:
> 
> https://developer.mozilla.org/en-US/docs/Chrome_Registration#appversion

Yes. That's the way to go.

    
    

    
  
I guess this is dev-doc-needed since it changes the properties and attributes of tab, so setting that keyword.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/tab

Also setting addon-compat so Jorge you can decide if this should be mentioned further.
Keywords: addon-compat, 
          
            dev-doc-needed

    
    

    
  
(In reply to Pardal Freudenthal (:ShareBird) from comment #76)
> (In reply to George Wright (:gw280) from comment #75)
> > I suggest using the chrome registration system to tie specific versions of
> > the CSS to the appversion:
> > 
> > https://developer.mozilla.org/en-US/docs/Chrome_Registration#appversion
> 
> Yes. That's the way to go.

You're correct. My library just helps addons which have too many CSS files to double for Firefox versions.

    
  
Depends on: 1186850

    
    

    
  
Docs:

* new page documenting visuallyselected: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/visuallyselected
* updated the old page on selected: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/selected
* new page outlining how tab selection works in multiprocess Firefox: https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Tab_selection_in_multiprocess_Firefox

Please let me know if this is correct and sufficient. (And thanks for your very informative blog post, which I raided extensively for this.)
Flags: needinfo?(gwright)

    
    

    
  
It might be worth stating what the timeout is (originally 300ms, currently 400ms after UX reviewed it), but apart from that it looks good! Thanks for this!
Flags: needinfo?(gwright)
Depends on: 1297157

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: