Bug 104419 - [CSS Exclusions] shape-inside layout fails to adjust first line correctly for writing-mode: vertical-rl
Summary: [CSS Exclusions] shape-inside layout fails to adjust first line correctly for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-07 16:54 PST by Hans Muller
Modified: 2012-12-18 11:09 PST (History)
3 users (show)

See Also:


Attachments
Test case. (1.16 KB, text/html)
2012-12-07 16:55 PST, Hans Muller
no flags Details
Patch (16.31 KB, patch)
2012-12-13 15:00 PST, Hans Muller
krit: review-
krit: commit-queue-
Details | Formatted Diff | Diff
Patch (19.30 KB, patch)
2012-12-17 09:00 PST, Hans Muller
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (19.30 KB, patch)
2012-12-17 16:34 PST, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 2012-12-07 16:54:57 PST
See the attached example.  The vertical-rl case fails.
Comment 1 Hans Muller 2012-12-07 16:55:50 PST
Created attachment 178307 [details]
Test case.
Comment 2 Hans Muller 2012-12-13 15:00:18 PST
Created attachment 179348 [details]
Patch

ExclusionShapes no longer maintain a private "internal" coordinate system, they're now defined in logical coordinates. The createExclusionShape() method now handles the one-time conversion from physical to logical coordinates.
Comment 3 Dirk Schulze 2012-12-17 08:46:01 PST
Comment on attachment 179348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179348&action=review

Fix looks ok. Looks like you forgot the ChangeLogs :).

> Source/WebCore/rendering/ExclusionShape.cpp:68
> +static inline FloatRect physicalRectToLogical(const FloatRect& r, float logicalBoxHeight, WritingMode writingMode)

s/r/rect/

> Source/WebCore/rendering/ExclusionShape.cpp:77
> +static inline FloatPoint physicalPointToLogical(const FloatPoint& p, float logicalBoxHeight, WritingMode writingMode)

s/p/point/

> Source/WebCore/rendering/ExclusionShape.cpp:86
> +static inline FloatSize physicalSizeToLogical(const FloatSize& s, WritingMode writingMode)

s/s/size/

> Source/WebCore/rendering/ExclusionShape.cpp:145
> +        exclusionShape->m_boundingBox = FloatRect(logicalCenter - logicalRadii, logicalRadii + logicalRadii);

logicalRadii + logicalRadii seems to be inconsistent with radius * 2 that you used previously. :)
Comment 4 Hans Muller 2012-12-17 09:00:05 PST
Created attachment 179755 [details]
Patch

Added the ChangeLog, made the suggested changes.
Comment 5 Build Bot 2012-12-17 10:46:59 PST
Comment on attachment 179755 [details]
Patch

Attachment 179755 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15366809

New failing tests:
fast/frames/sandboxed-iframe-attribute-parsing.html
Comment 6 Hans Muller 2012-12-17 16:34:04 PST
Created attachment 179830 [details]
Patch

Resubmitting original patch since the Mac failure that caused the commit-bot to reject this patch also failed without this patch.
Comment 7 Dirk Schulze 2012-12-18 08:20:23 PST
Comment on attachment 179830 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179830&action=review

LGTM. Just a snippet. R=me

> Source/WebCore/rendering/ExclusionShape.cpp:145
> +        exclusionShape->m_boundingBox = FloatRect(logicalCenter - logicalRadii, logicalRadii + logicalRadii);

2 * radii
Comment 8 WebKit Review Bot 2012-12-18 11:09:47 PST
Comment on attachment 179830 [details]
Patch

Clearing flags on attachment: 179830

Committed r138043: <http://trac.webkit.org/changeset/138043>
Comment 9 WebKit Review Bot 2012-12-18 11:09:51 PST
All reviewed patches have been landed.  Closing bug.