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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 84931: Patch
https://bugs.webkit.org/attachment.cgi?id=84931&action=review

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

I wonder if there are any code paths that reuse the same style object and
change the position. I couldn’t find any, but if they existed we’d want to
clear this flag.

There might be some obscure case involving an SVG element.

> Source/WebCore/css/CSSStyleSelector.cpp:1939
> +	       style->setPositionWasRelative(true);
>	       style->setPosition(StaticPosition);

I noticed similar code for frame and frameset elements. Perhaps the bit should
be set there too.

> Source/WebCore/rendering/RenderObject.cpp:2618
> -		       (!curr->isPositioned() && !curr->isRelPositioned() &&
!curr->isBody()))) {
> +		       (!curr->isPositioned() && !(curr->isRelPositioned() ||
curr->style()->positionWasRelative()) && !curr->isBody()))) {

It’s a little awkward here to use:

    !a && !(b || c) && !d

Instead of:

    !a && !b && !c && !d

Or:

    !(a || b || c || d)

If you instead added a single inline helper function that returned “is or was
relative” then I could see keeping it the way you have it here.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:112
> +    // WebKit doesn't support position:relative for several table elements
and overwrites the style internally when position:relative is encountered.
> +    // This flag preservers the fact that the position attribute was
originally relative, and was subsequently "corrected".

Typo: “preservers”.

I’m not sure this comment strikes the right tone. The use of scare quotes
around the word corrected and the words “doesn’t support” make it sound like
the handling of relative positioning is a bug. Unless that is the case I would
prefer not to give that impression.