573241 - Clicking "New Tab Button " creates a new tab with the label 'New Tab' in a faint gray and/or blank

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100619 Minefield/3.7a6pre ID:20100619040308
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100619 Minefield/3.7a6pre ID:20100619040308

Clicking "New Tab Button " creates a new tab with the label 'New Tab' in a faint gray and/or blank label.


This happens with default theme and/or builtin persona theme(such as "Dark Fox").

Reproducible: Always

Steps to Reproduce:
1. Start Minfield with new profile
2. Click "New Tab Button "


Actual Results:
 creates a new tab with the label 'New Tab' in a faint gray and/or blank .

Expected Results:
 The label 'New Tab' should be painted properly.

Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/88fa0b783306
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre)
Gecko/20100618 Minefield/3.7a6pre ID:20100618085618


Fails:
http://hg.mozilla.org/mozilla-central/rev/d4156799e66a
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre)
Gecko/20100617 Minefield/3.7a6pre ID:20100618100908

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88fa0b783306&tochange=d4156799e66a

i narrowed down regression window in local build, 
Revert to 8d309d7e0746 : the problem is reproduced
Revert to dee1e84a95aa : the problem is reproduced
Revert to d594bc58ca6b: the problem is   *NOT* reproduced
Revert to 9ec74c8e5690 : the problem is  *NOT* reproduced

Candidate:
Bug 494117 - Don't rerun selector matching on the whole subtree unless we need
to

Comment 1

[Alice0775 White]

[STR2]
1. Start Minefield with new profile
2. Position "mouse pointer" at the right side of "New Tab Button"(not on the button)
3. Ctrl+T

[STR3]
1. Start Minefield with new profile
2. Middle click "New Tab Button "

Gray label appears.

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

The problem does not seem to occur if you reposition the + (add tab) button to the left side of the tab-toolbar.

Comment 3

[Tiago Sá]

On my side, it doesn't happen if, as I click the new tab button (on the default position), I immediately move the cursor horizontally. If I move it vertically, the blank tab appears. Always. If I don't move it (the cursor), it appears most of the times.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that some comments about this bug were originally made in bug 573221, but then this bug was separated from it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, I can reproduce on Mac (though not reliably), but NOT on Linux...

I'll see what I can dig up.  I wonder whether this is relevant:

WARNING: ElementData returned null: file /Users/bzbarsky/mozilla/css-frameconst/mozilla/layout/style/nsTransitionManager.cpp, line 232

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and I'm off tomorrow, so will likely look Tuesday morning.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, that warning appears even if the bug is not present.  And the bug disappears if I make PresShell::RestyleForAnimation do a RestyleSubtree...  Which indicates that there's a bug elsewhere too.

Is the tab title supposed to fade in or something?  If so, can someone point me to the transition controlling that?

Comment 9

[Dão Gottwald [:dao]]

Yes, it's supposed to fade in, see lines 36 to 46: http://hg.mozilla.org/mozilla-central/annotate/c7eaf9387b1f/browser/base/content/browser.css#l36

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, I see.  What's really weird here is that it looks like the transition sometimes stops before completing.  Which is very odd...

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, I logged the classes of things coming through RestyleForAnimation.  One interesting thing is that I see a lot of:

  tab-icon-image
  tabbrowser-tab
  tab-close-button

over and over.  Note conspicuous lack of tab-text in that output.  They all suddenly pop in if I move focus to a different window, though.

If I only do a subtree restyle for tabbrowser-tab, the bug disappears.

Also, the warning I mention in comment 6 appears a _lot_ more with the self restyle on tabbrowser-tab.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, at least some of those warnings are coming from a ElementTransitionsStyleRule::MapRuleInfoInto call on a rule which has already had Disconnect called.  And Disconnect got called because in nsTransitionManager::ConsiderStartingTransition both the old and new style context had opacity 0, so we destroyed the aElementTransitions.

It looks like we ended up with ReResolveStyleContext called with aRestyleHint == 0 (which means "just reparent the style context" now), but our style context included a transition rule.  Before the changes in bug 494117 we'd get a style context which would not include the transition rule, and then this block in ConsiderStartingTransition made us do nothing interesting, I _think_:

    if (oldPT.mEndValue == pt.mEndValue) {
      // If we got a style change that changed the value to the endpoint
      // of the currently running transition, we don't want to interrupt
      // its timing function.
      // WalkTransitionRule already called RestyleForAnimation.
      return;
    }

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows a transition getting interrupted incorrectly

The color change makes the transition jump directly to opaque in this case, so it's not _quite_ what the tabs are hitting, but I think related.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

This fixes the attached testcase for me, and seems to fix the bug as reported.  Certainly I no longer see the warnings from comment 6 (maybe this was the cause of that situation all along?) and I see animation restyles happening on tab-text as expected instead of stopping early.

David, if you think this is the wrong thing for some ReparentStyleContext consumers (which ones?) I can just add a new API for ReResolveStyleContext to use that does this.  But it seems like all ReparentStyleContext callers would want this behavior, at first blush...

I still need to sort out how to write transition tests (and hence how to turn the attached testcase into a failing automated test).

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we call ReParentStyleContext during reresolve now?  I thought we were actually doing all the reparenting within ReResolve, but maybe I missed something.

In any case, skipping transition rules should be conditional on:

PresContext()->IsProcessingRestyles() &&
!PresContext()->IsProcessingAnimationStyleChange()

So r=dbaron if both of the while loops you added are conditional on that condition.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 452642 [details] [diff] [review]
Possible patch

r=dbaron with the above change, assuming it also makes sense to you

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though I'd still like to know how we end up in ReParentStyleContext while processing a style change.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Do we call ReParentStyleContext during reresolve now?

Yes, that was the major change in bug 494117 (the rest was just making it possible to figure out _when_ to call ReparentStyleContext).  See https://bugzilla.mozilla.org/attachment.cgi?id=447818&action=diff#a/layout/base/nsFrameManager.cpp_sec5 and https://bugzilla.mozilla.org/attachment.cgi?id=447818&action=diff#a/layout/base/nsFrameManager.cpp_sec8

> In any case, skipping transition rules should be conditional on:

Hmm...  I _think_ that makes sense, but I'll need to sleep on it.  I can't very well watch the tree tonight anyway at this point.  ;)

Comment 19

[Boris Zbarsky [:bzbarsky]]

In particular, if I have a span transitioning opacity and it gets pulled up into the first line of a div, should its transition get cut off?  Then again, it gets cut off even with the attached patch (but not in Safari, fwiw).  I'm not quite sure why it gets cut off in that situation.  The comment 6 warning does NOT appear in that case, though.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase per comment 19

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #18)
> > Do we call ReParentStyleContext during reresolve now?
> 
> Yes, that was the major change in bug 494117 (the rest was just making it
> possible to figure out _when_ to call ReparentStyleContext).

Oh, right.  I was confusing nsStyleSet::ReparentStyleContext with the no-longer-existing nsFrameManager::ReparentStyleContext, and then thinking your change here was in the latter rather than the former.  But I'd already confirmed when reviewing bug 494117 that we weren't calling nsFrameManager::ReparentStyleContext (which reparented trees, not individual contexts), because I'd discovered it didn't exist anymore.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And, actually, I think you *also* need to call RestyleForAnimation on any element where you remove rules in this manner, since nsTransitionManager::WalkTransitionRule does that.

Comment 23

[Damon Sicore (:damons)]

Doesn't sound like a beta *1* blocker to me.  Yes, must fix, so blocking, but not beta 1.  Yell if you disagree.

Comment 24

[Boris Zbarsky [:bzbarsky]]

This bug is generally breaking transitions in various ways; the tabs just show it particularly well.

I also fully expect to have a patch for this ready by am tomorrow.   I do think this should block b1 unless we're shipping b1 tomorrow morning or tonight.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ah, the reason for the lack of transition when moving up inside the first-line is this hunk in ElementTransitionsStyleRule::MapRuleInfoInto:

223       nsStyleContext *contextParent = aRuleData->mStyleContext->GetParent();
224       if (contextParent && contextParent->HasPseudoElementData()) {
225         // Don't apply transitions to things inside of pseudo-elements.
226         // FIXME (Bug 522599): Add tests for this.
227         return;
228       }

So I'm not going to worry about that for now, I guess.

Given that, I suppose the IsProcessingRestyles() part of the condition in comment 15 makes sense.

Comment 22 makes sense too, though getting the right element is a pain in the behind.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments; still needs tests

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w of the same

I'd love ideas on how to exercise the RestyleForAnimation() bit, since the one testcase I have in this bug doesn't seem to fail without it...

Comment 28

[Boris Zbarsky [:bzbarsky]]

Review note:  The refactoring of the element getter depends on the various assertions in ReResolveStyleContext not firing.  Given that, I believe it is equivalent to the current code.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with tests

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w with tests

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 453071 [details] [diff] [review]
diff -w with tests

r=dbaron

Comment 32

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/54d47e708c9c