Bug 96597 – Float block's logical top margin is illegal in vertical writing mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Yuki Sekiguchi]

Created attachment 163767 [details]
reproduced content

Float block's logical top margin is illegal in vertical writing mode.

This bug is reproduced by the attached HTML content.

Blue block have offset to left although the style is margin: 50px.
The block should be at the center of black border.

[Yuki Sekiguchi]

Created attachment 163768 [details]
Patch

[WebKit Review Bot]

Comment on attachment 163768 [details]
Patch

Attachment 163768 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13832638

New failing tests:
fast/multicol/vertical-rl/float-multicol.html

[Yuki Sekiguchi]

Created attachment 164017 [details]
Add need rebaseline for chromium.

[Yuki Sekiguchi]

fast/multicol/vertical-rl/float-multicol.html regression is intentional.
I watch it on Chromium Linux and it works well.
I add need rebaseline for Chromium.

[Yuki Sekiguchi]

Created attachment 164050 [details]
Rebase

[Hajime Morrita]

Comment on attachment 164050 [details]
Rebase

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

Looks good. Please adress my nitpicky comment.

> Source/WebCore/ChangeLog:8
> +        Float block's logical top margin is illegal in vertical writing mode.

Please don't repeat this.

> LayoutTests/ChangeLog:9
> +        This test's block should be at the center of black border

This explanation should be in the test so that test is more readable.

[Yuki Sekiguchi]

Created attachment 171766 [details]
Fix test indications

[Yuki Sekiguchi]

Thank you for reviewing, Morrita-san!

(In reply to comment #6)
> > Source/WebCore/ChangeLog:8
> > +        Float block's logical top margin is illegal in vertical writing mode.
> 
> Please don't repeat this.
Removed.

> > LayoutTests/ChangeLog:9
> > +        This test's block should be at the center of black border
> 
> This explanation should be in the test so that test is more readable.
Moved this description to test.
I changed "inside" to "at the center of" in vertical-float-margin.html.

[WebKit Review Bot]

Comment on attachment 171766 [details]
Fix test indications

Rejecting attachment 171766 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
atching file LayoutTests/fast/writing-mode/vertical-float-margin-expected.html
patching file LayoutTests/fast/writing-mode/vertical-float-margin.html
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 4162.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Hajime Mor..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14717669

[Yuki Sekiguchi]

Created attachment 172896 [details]
Rebase

[Yuki Sekiguchi]

Created attachment 178672 [details]
Rebase

[Yuki Sekiguchi]

Created attachment 178687 [details]
Rebase

[WebKit Review Bot]

Comment on attachment 178687 [details]
Rebase

Attachment 178687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15257440

New failing tests:
fast/text/emphasis-vertical.html

[Yuki Sekiguchi]

Created attachment 178943 [details]
Fix TestExpectations to need rebaseline

[Yuki Sekiguchi]

Morrita-san, could you review it?

Rebasing for commit queue make a chromium test fail. I add it to TestExpectations.

[Yuki Sekiguchi]

Created attachment 181617 [details]
Patch

[Yuki Sekiguchi]

Morrita-san, could you review it?

You give lgtm to this patch.
However, rebasing for commit queue make a chromium test fail.
I add it to TestExpectations.

[Hajime Morrita]

Comment on attachment 181617 [details]
Patch

Sure.

Just FYI...

You can upload patch with
- replacing NOBODY by reviewers name,
- leaving r field empty and
- just set cq field. 

Doing it tells that the patch is already reviewed and ready to commit.
Any committer (not reviewer) can check it in.
In #webkit IRC channel, it is pretty normal to ask to commit such patches.

[Yuki Sekiguchi]

Thank you for your advice!
I try it at next time that commit queue is rejected.

[WebKit Review Bot]

Comment on attachment 181617 [details]
Patch

Clearing flags on attachment: 181617

Committed r139040: <http://trac.webkit.org/changeset/139040>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

There are two entries for fast/text/emphasis-vertical.html in TestExpectations:

# Needs rebaseline. Current expectation is wrong because margin of body don't collapse with margin of 1st child.
webkit.org/b/96597 fast/text/emphasis-vertical.html [ ImageOnlyFailure ]
# WebKit roll 74255:74308
crbug.com/67416 [ Win Mac Android ] fast/text/emphasis-vertical.html [ Failure ImageOnlyFailure ]

one overrides the other.  Do you intend to fix?

[noel gordon]

From http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/32571/steps/layout-test/logs/stdio

Parsing expectations ...
--lint-test-files warnings:
...
LayoutTests\platform\chromium\TestExpectations:4364 More specific entry for fast/text/emphasis-vertical.html on line LayoutTests\platform\chromium\TestExpectations:4364 overrides line LayoutTests\platform\chromium\TestExpectations:2589. fast/text/emphasis-vertical.html
...

[Yuki Sekiguchi]

(In reply to comment #22)
> There are two entries for fast/text/emphasis-vertical.html in TestExpectations:
> 
> # Needs rebaseline. Current expectation is wrong because margin of body don't collapse with margin of 1st child.
> webkit.org/b/96597 fast/text/emphasis-vertical.html [ ImageOnlyFailure ]
> # WebKit roll 74255:74308
> crbug.com/67416 [ Win Mac Android ] fast/text/emphasis-vertical.html [ Failure ImageOnlyFailure ]
> 
> one overrides the other.  Do you intend to fix?

Sorry for late reply. I missed this comment.
Stephen fixed this at r139467:
http://trac.webkit.org/changeset/139467

Thank you, Stephen.