313642 - Remove js width and height calculation hacks in marquee code which are not needed anymore

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

This is a follow-up from bug 163505. Now that bug is fixed it makes it possible to clean some of the code up in xbl-marquee.xml.
By the way, I forgot that IE6 supports the bgcolor attribute (but not the background attribute) on the marquee. I would like to fix that here too in this bug.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Well, the patch isn't working on the second part of this testcase. With the patch, it shows the background-image, but it should not.
How can I remove support for the background attribute, but keep bgcolor?

Comment 3

[Boris Zbarsky [:bzbarsky]]

To do that, you need to not use nsGenericHTMLElement::MapBackgroundAttributesInto.  You probably need to factor it into two functions (MapBackgroundInto and MapBGColorInto), with MapBackgroundAttributesInto calling both functions, and call only MapBGColorInto from this code.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch2a

Ok, thanks.
I'm splitting the previous patch in two. This is the part without the bgcolor stuff.
Doron, if you could take a look at this too, that would be much appreciated!

Comment 5

[Doron Rosenberg (IBM)]

looks fine

Comment 6

[Martijn Wargers (dead)]

Attachment: patch3

Ok, something like this, I guess.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200784 [details] [diff] [review]
patch3

>Index: content/html/content/src/nsGenericHTMLElement.h
>+  static void MapBackgroundInto(const nsMappedAttributes* aAttributes,
>+                                          nsRuleData* aData);

Fix indent, please.

>+  static void MapBGColorInto(const nsMappedAttributes* aAttributes,
>+                                          nsRuleData* aData);
>+  /**

And here.

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::MapBackgroundInto(const nsMappedAttributes* aAttributes,
>                                                   nsRuleData* aData)

And here.

>Index: content/html/content/src/nsHTMLDivElement.cpp
> nsHTMLDivElement::IsAttributeMapped(const nsIAtom* >+      sBackgroundAttributeMap,

That claims that "background" is mapped.  It's not.  You might want to just create a separate sBGAttributeMap for use here or something.

>Index: layout/style/xbl-marquee/xbl-marquee.xml
>-      <property name="width">

This makes the "width" getter behave very very differently than it used to.  Why is this change OK?  What happens for auto-width marquees?

>-              // "", 0 and empty height should default to 200px

What happened to that?  That is, I don't think we do this anymore; why not?

doron, please take a close look at the xml changes?  I don't see how they could possibly be "fine", but I'm not really qualified to review them to start with.

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase2, using auto-width marquees

> This makes the "width" getter behave very very differently than it used to. 
> Why is this change OK?  What happens for auto-width marquees?
This testcase renders the same for me in Mozilla1.7, Firefox trunk and debug trunk build with patch, so nothing broken there with the patch. (Note, IE6 is different for the 3rd and 4th case, with a very small, almost 0 width, marquee)
I'm removing also the only consumer of the width property in xbl-marquee.xml, since I think it isn't necessary (anymore). That's this line in the patch:
-          this.outerDiv.style.width = this.width;

But to be frank, I don't know why it is OK to remove that code, since I don't know why it is in there, the first place. I tried to remove a large part of that code also in bug 235564. Doron insisted it should stay in. But I don't know of any sites/testcases/examples that gets fixed by that code. And to me, it looks like code that doesn't do anything, so that's why I keep getting removing it.
I haven't seen anything that regressed when removing that code.
I can leave it in if you prefer.

> >-              // "", 0 and empty height should default to 200px
> What happened to that?  That is, I don't think we do this anymore; why not?
Well, this is also done here:
http://lxr.mozilla.org/seamonkey/source/layout/style/html.css#489
But since attribute mapping wasn't working, the js code was also necessary, but currently not anymore.

Comment 9

[Martijn Wargers (dead)]

Attachment: patch4

This patch should address the rest of your comments.
I've called the thing sBackgroundColorAttributeMap , it seems to me more in line with the rest.
Perhaps I should change MapBGColorInto into MapBackgroundColorInto?

Comment 10

[Doron Rosenberg (IBM)]

Comment on attachment 200825 [details] [diff] [review]
patch4

r=me for the marquee parts.

Hopefully if any stuff that was there for a reason that was removed will appear in bug reports :)

Comment 11

[Martijn Wargers (dead)]

Comment on attachment 200825 [details] [diff] [review]
patch4

I have tested this on at least 20 url's with marquees in it and appr. 50 bugzilla marquee testcases.
I didn't find any regressions because of this patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200825 [details] [diff] [review]
patch4

sr=bzbarsky.  Thanks for the clarifications, Martijn!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 14

[Martijn Wargers (dead)]

Attachment: stuff I forgot

Eek!
I forgot to remove this stuff, this can also safely be removed, it is needed to fix bug 162980.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200964 [details] [diff] [review]
stuff I forgot

Checked this in.  Thanks, Martijn!

Comment 16

[Martijn Wargers (dead)]

Thank you (and Doron). Marquee in Mozilla is still not perfect ;) , so if I could bother you people with some more patches, that would be great.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Bring them on.  ;)