Bug 128631

Summary: [CSS Shapes] shape-outside does not properly handle different writing modes
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: Layout and RenderingAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, kondapallykalyan, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128685    
Bug Blocks: 98664    
Attachments:
Description Flags
Do not land this patch
none
Patch
none
Patch
none
Patch
none
Patch none

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.