163505 - marquee stylistic attributes should be handled through attribute mapping

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

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

marquee's stylistic attributes should be handled through the attribute mapping
code (nsIStyledContent::GetAttributeMappingFunction, etc.) rather than through
the current JS hacks.  This would fix things such as the second half of bug
163031, which is that the 'width' property on the marquee element doesn't work
right, but instead is overridden (or partly so) by the width attribute.  It
would also make things a lot cleaner and fix a bunch of bugs related to
attribute handling.

Having said that, I still think it's pretty low priority.

Comment 1

[José Jeria]

Attachment: Testcase

Testcase showing that width does not work when set with css.

Comment 2

[José Jeria]

Attachment: Testcase 2

This testcase has width set as an html attribute. Note that the scroll area of
the Marquee is 400 px, but that the marquees width seems to be 100%.

Comment 3

[Madhur Bhatia]

Attachment: testcase 1 and testcase 2 combined

this shows the behaviour of marquee when the width is assigned as an HTML
attribute vs. a CSS property

Comment 4

[Boris Zbarsky [:bzbarsky]]

To doron.  Perhaps the XBL should implement nsIStyledContent?  Can XBL implement
non-IDLified interfaces?

Comment 5

[Martijn Wargers (dead)]

Attachment: patch

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase3

Comment 7

[Martijn Wargers (dead)]

Attachment: patch2

Boris had a quick look at the previous patch, he had this to say:
bz	{#developers} mw22: so you want IsAttributeMapped to match
GetAttributeMappingFunction
bz	{#developers} mw22: that is, use a different map there for the
different nodeinfos
bz	{#developers} mw22: other than that, looks fine to me..
He also said to remove gutter and cols stuff.

Comment 8

[Doron Rosenberg (IBM)]

Comment on attachment 200297 [details] [diff] [review]
patch2

I really don't feel comfortable reviewing content code.  bz might be a better
person.

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 200297 [details] [diff] [review]
patch2

Ok, thanks. Asking Boris for review, then.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200297 [details] [diff] [review]
patch2

>Index: content/html/content/src/nsHTMLDivElement.cpp
> nsHTMLDivElement::IsAttributeMapped(const nsIAtom* aAttribute) const

>   static const MappedAttributeEntry* const map[] = {
>-    sDivAlignAttributeMap,
>     sCommonAttributeMap
>   };
>-  
>   return FindAttributeDependence(aAttribute, map, NS_ARRAY_LENGTH(map));

replace all this with |return
nsGenericHTMLElement::IsAttributeMapped(aAttribute)|, please.  That will ensure
that this is consistent with GetAttributeMappingFunction even if the generic
changes.

Apart from that, looks great.

Comment 11

[Martijn Wargers (dead)]

Attachment: patch3

Ok, done that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.  Is there a followup bug to remove the JS hacks?

Comment 13

[Martijn Wargers (dead)]

I've filed bug 313642 for that.