502567 - [FIX]new.music.yahoo.com, Top charts list is empty

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Chwiej Pawel]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090703 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090703 Minefield/3.6a1pre




Reproducible: Always

Steps to Reproduce:
1. Visit new.music.yahoo.com
2. Look at the Top Charts on the left.
Actual Results:  
List is empty, check screenshot.

Expected Results:  
Top Charts list shouldn't be empty.

Comment 1

[Chwiej Pawel]

Attachment: screenshot

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows Vista, latest trunk. This bug is not present on Shiretoko.
Regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4f196841ba1a&tochange=38c24ab6f2a0

Comment 3

[Matthias Versen [:Matti]]

CC the 3 developers from checkins in the timeframe

Comment 4

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

Has to be bzbarsky's change, since smichaud's was Mac-only and mine was test-only.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 472501.  ShouldBuildChildFrames() is returning false for some reason.  Looking into that now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  So those links have class="ymusic-ellipsis" and the site has these style rules:

  .ymusic-ellipsis{
    -moz-binding:url("/css/ellipsisxul.xml#ellipsis");
  }
  .ymusic-ellipsis {
    display:block;
    overflow:hidden;
    width:100%;
    white-space:nowrap;
    text-overflow:ellipsis;
    -o-text-overflow:ellipsis;
  }

That binding looks like this:

  <binding id="ellipsis">
    <content>
      <xul:window>
        <xul:description crop="end" xbl:inherits="value=xbl:text">
          <children/>
        </xul:description>
      </xul:window>
    </content>
  </binding>

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: XUL testcase that has similar behavior in Firefox 3

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the relevant thing here is that nsXBLPrototypeBinding::ShouldBuildChildFrames returns false if and only if mAttributeTable has an entry for xbl:text.  Which is precisely the situation only when "xbl:text" appears on the right hand side of '=', right?

Now the question is why we even have this code...  There are no uses of xbl:text on the RHS of '=' anywhere in our tree, and the blame for this function is no useful:

  1.12 <[e-mail]>  2000-12-07 02:11
  Big XBL landing. Fixes numerous XBL bugs. a=ben

We should either remove this ShouldBuildChildFrames check altogether, or fix how it works if it should somehow work differently, or move this bug to tech evangelism because the site is just broken (which it is, no matter what).

I'd rather prefer removing this check altogether, myself; I don't see how it can possibly make sense now that I've figured out what it really does.

Comment 9

[Martijn Wargers (dead)]

Attachment: testcase

Comment 10

[Martijn Wargers (dead)]

Oh, sorry, didn't notice you already had a testcase.

Comment 11

[[e-mail]]

I guess xbl:inherits="value=xbl:text" was implemented for symmetry with xbl:inherits="xbl:text=value" (which we do of course use).

For the specific case of <xul:description> (or label) I doubt it matters what the XBL code does since as soon as the element gains a value attribute it ignores its children anyway.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sure.  My point is that if a node has a binding that has any anonymous element with xbl:inherits="value=xbl:text" then we currently treat that node as having no kids.  That is, none of its kids, explicit or anonymous, are rendered.  We seem to do this quite purposefully.  Is there a reason to keep doing it?

Comment 13

[[e-mail]]

Well if you're going to map the text content to an attribute then I don't see why you'd want the text content to get rendered independently...

Comment 14

[Boris Zbarsky [:bzbarsky]]

It doesn't just skip rendering "the text content".  It skips rendering all kids, text or not, including the one with the attribute.  If you just don't want to render the explicit DOM kids, you can give display:none to the parent of the <children/> node, right?

Given lack of use cases in our tree, I'm tempted to remove this code, assuming my analysis in comment 8 paragraph 1 is correct....

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Go for it!

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/eef814b58a9c

Comment 19

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/efbdf6ac87e9

Comment 20

[[not reading bugmail]]

Verified fixed Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre (.NET CLR 3.5.30729)

Comment 21

[Tony Chung [:tchung]]

Also verified on mac 1.9.2 branch.
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20091008 Namoroka/3.6b1pre