Bug 128631 - [CSS Shapes] shape-outside does not properly handle different writing modes
Summary: [CSS Shapes] shape-outside does not properly handle different writing modes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
URL:
Keywords:
Depends on: 128685
Blocks: 98664
  Show dependency treegraph
 
Reported: 2014-02-11 15:45 PST by Bem Jones-Bey
Modified: 2014-02-19 07:14 PST (History)
5 users (show)

See Also:


Attachments
Do not land this patch (29.33 KB, patch)
2014-02-11 15:46 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (48.50 KB, patch)
2014-02-14 14:26 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (48.53 KB, patch)
2014-02-18 13:16 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (48.52 KB, patch)
2014-02-18 13:17 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (48.61 KB, patch)
2014-02-18 15:33 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bem Jones-Bey 2014-02-11 15:45:13 PST
ShapeOutsideInfo::updateDeltasForContainingBlockLine doesn't properly convert from the writing mode and coordinate space of the containing block to and from the coordinate space of the shape. Some of this is due to the confusing names of the variables involved, some is due to the fact that the logicalLeftOffset(), logicalTopOffset() and other methods in ShapeInfo use the writing mode of the float, not of the parent. This bug tracks fixing all of these issues.
Comment 1 Bem Jones-Bey 2014-02-11 15:46:13 PST
Created attachment 223909 [details]
Do not land this patch

Large patch for prelimary perusal. Real patch would be split into smaller pieces and include things like tests.
Comment 2 Bem Jones-Bey 2014-02-14 14:26:40 PST
Created attachment 224255 [details]
Patch

I tried breaking this up into multiple patches, but had trouble coming up with tests for each piece.
Comment 3 Bem Jones-Bey 2014-02-18 11:44:25 PST
Comment on attachment 224255 [details]
Patch

I've gotta add a couple of ASSERT_NOT_REACHED to make EFL happy.
Comment 4 Dave Hyatt 2014-02-18 11:51:14 PST
Comment on attachment 224255 [details]
Patch

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

r=me with a rename suggestion.

> Source/WebCore/rendering/RenderBoxModelObject.h:121
> +    virtual LayoutUnit borderWidth() const { return borderLeft() + borderRight(); }
> +    virtual LayoutUnit borderHeight() const { return borderTop() + borderBottom(); }

I would suggest naming these horizontalBorderExtent and verticalBorderExtent. I dislike using terms like "width" since it could be confused with the actual border-width CSS name, i.e., a person new to this code would think the method was returning the pixel width of a single border.

I think renaming all of the other functions would be good too, i.e., marginWidth = horizontalMarginExtent, marginHeight = verticalMarginExtent, etc.

borderAndPaddingWidth could be horizontalBorderAndPaddingExtent, and then borderAndPaddingHeight could be verticalBorderAndPaddingExtent.
Comment 5 Bem Jones-Bey 2014-02-18 13:16:41 PST
Created attachment 224536 [details]
Patch

Will do renames in a followup patch
Comment 6 Bem Jones-Bey 2014-02-18 13:17:58 PST
Created attachment 224537 [details]
Patch

forgot to update reviewer
Comment 7 Bem Jones-Bey 2014-02-18 15:33:39 PST
Created attachment 224560 [details]
Patch

My ability to read error messages is apparently a problem. I need a return statement to shut that EFL compiler up!
Comment 8 WebKit Commit Bot 2014-02-19 07:14:46 PST
Comment on attachment 224560 [details]
Patch

Clearing flags on attachment: 224560

Committed r164363: <http://trac.webkit.org/changeset/164363>
Comment 9 WebKit Commit Bot 2014-02-19 07:14:49 PST
All reviewed patches have been landed.  Closing bug.