[webkit-reviews] review denied: [Bug 52535] offsetLeft is wrong for elements inside a TD whose style is set to position:relative : [Attachment 84273] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 52535: offsetLeft is wrong for elements inside a TD whose style is set to
position:relative
https://bugs.webkit.org/show_bug.cgi?id=52535

Attachment 84273: Patch
https://bugs.webkit.org/attachment.cgi?id=84273&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84273&action=review

Great to tackle this. This fix is wrong, though.

> Source/WebCore/rendering/RenderObject.cpp:2161
> +	 // If relative positioning is already set, then no need to look at the

> +	 // raw style.

Should put that comment on one line.

> Source/WebCore/rendering/RenderObject.cpp:2169
> +	 CSSStyleDeclaration* rawStyle = toElement(node())->style();
> +	 RefPtr<CSSValue> position =
rawStyle->getPropertyCSSValue(CSSPropertyPosition);
> +	 if (position.get() && position->cssText() == "relative")
> +	     return true;
> +	 return false;

This works only if the style is set with a style attribute, not when it’s set
with a stylesheet. You should add a test case where position:relative is set in
the stylesheet, perhaps based on class or ID, and that test case will fail.

If we need this information we will probably have to put it in the RenderStyle.
Going back to the DOM doesn’t seem workable because there’s no practical way to
go back to all the various style declarations that could have influenced the
style of the element.

The style declaration that comes from the style attribute for an element is not
called the “raw style” and coining that term here is not a good idea. That one
style declaration contains only a small part of what determines the style of an
element.

Should not need the call to “.get()” here. And if (x) return true; return
false; could just be replaced by return.

Getting the cssText is not the right way to check a CSSValue. The right way is
more like this:

    static bool valueIsKeyword(CSSValue* value, int keyword)
    {
	return value && value->isPrimitiveValue() &&
static_cast<CSSPrimitiveValue*>(value)->getIdent() == keyword;
    }

    ...

    return
valueIsKeyword(rawStyle->getPropertyCSSValue(CSSPropertyPosition).get(),
CSSPropertyRelative);

But I think that’s beside the point since this is the wrong approach.

> Source/WebCore/rendering/RenderObject.h:405
> +    // WebKit doesn't support relative positioning for certain elements.
> +    // In such cases isRelPositioned() won't match the value set in the
original style declaration.
> +    // This function returns the value in the raw style declaration.

I think this is a confusing comment. It doesn’t make it clear if this lack of
support is a bug or a feature. Same comment as above about the term “raw
style”.