Closed Bug 573241 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b1
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: alice0775, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

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
[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.
The problem does not seem to occur if you reposition the + (add tab) button to the left side of the tab-toolbar.
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.
blocking2.0: --- → beta1+
Note that some comments about this bug were originally made in bug 573221, but then this bug was separated from it.
Duplicate of this bug: 573299
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
Oh, and I'm off tomorrow, so will likely look Tuesday morning.
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?
OK, I see.  What's really weird here is that it looks like the transition sometimes stops before completing.  Which is very odd...
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.
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;
    }
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.
Attached patch Possible patch (obsolete) — Splinter Review
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).
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #452642 - Flags: review?(dbaron)
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 on attachment 452642 [details] [diff] [review]
Possible patch

r=dbaron with the above change, assuming it also makes sense to you
Attachment #452642 - Flags: review?(dbaron) → review+
Though I'd still like to know how we end up in ReParentStyleContext while processing a style change.
> 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.  ;)
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.
(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.
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.
Doesn't sound like a beta *1* blocker to me.  Yes, must fix, so blocking, but not beta 1.  Yell if you disagree.
blocking2.0: beta1+ → beta2+
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.
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.
Attachment #452642 - Attachment is obsolete: true
Attached patch diff -w of the same (obsolete) — Splinter Review
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...
Attachment #452976 - Flags: review?(dbaron)
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.
Attached patch Now with testsSplinter Review
Attachment #452975 - Attachment is obsolete: true
Attachment #452976 - Attachment is obsolete: true
Attachment #452976 - Flags: review?(dbaron)
Attachment #453071 - Flags: review?(dbaron)
Comment on attachment 453071 [details] [diff] [review]
diff -w with tests

r=dbaron
Attachment #453071 - Flags: review?(dbaron) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/54d47e708c9c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Duplicate of this bug: 574107
Duplicate of this bug: 574615
You need to log in before you can comment on or make changes to this bug.