Bug 96597

Summary: Float block's logical top margin is illegal in vertical writing mode.
Product: WebKit Reporter: Yuki Sekiguchi <yuki.sekiguchi>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, morrita, noel.gordon, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
reproduced content
none
Patch
none
Add need rebaseline for chromium.
none
Rebase
none
Fix test indications
none
Rebase
none
Rebase
none
Rebase
none
Fix TestExpectations to need rebaseline
none
Patch none

Description Yuki Sekiguchi 2012-09-12 21:11:47 PDT
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.
Comment 1 Yuki Sekiguchi 2012-09-12 21:18:23 PDT
Created attachment 163768 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-12 23:24:17 PDT
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
Comment 3 Yuki Sekiguchi 2012-09-13 17:56:33 PDT
Created attachment 164017 [details]
Add need rebaseline for chromium.
Comment 4 Yuki Sekiguchi 2012-09-13 17:59:43 PDT
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.
Comment 5 Yuki Sekiguchi 2012-09-13 22:49:00 PDT
Created attachment 164050 [details]
Rebase
Comment 6 Hajime Morrita 2012-10-30 03:35:35 PDT
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.
Comment 7 Yuki Sekiguchi 2012-10-31 21:39:00 PDT
Created attachment 171766 [details]
Fix test indications
Comment 8 Yuki Sekiguchi 2012-10-31 21:42:35 PDT
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.
Comment 9 WebKit Review Bot 2012-11-04 17:19:44 PST
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
Comment 10 Yuki Sekiguchi 2012-11-07 17:18:49 PST
Created attachment 172896 [details]
Rebase
Comment 11 Yuki Sekiguchi 2012-12-10 17:16:56 PST
Created attachment 178672 [details]
Rebase
Comment 12 Yuki Sekiguchi 2012-12-10 18:57:00 PST
Created attachment 178687 [details]
Rebase
Comment 13 WebKit Review Bot 2012-12-10 20:39:17 PST
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
Comment 14 Yuki Sekiguchi 2012-12-11 18:52:22 PST
Created attachment 178943 [details]
Fix TestExpectations to need rebaseline
Comment 15 Yuki Sekiguchi 2012-12-11 18:59:29 PST
Morrita-san, could you review it?

Rebasing for commit queue make a chromium test fail. I add it to TestExpectations.
Comment 16 Yuki Sekiguchi 2013-01-07 18:14:23 PST
Created attachment 181617 [details]
Patch
Comment 17 Yuki Sekiguchi 2013-01-07 21:04:15 PST
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.
Comment 18 Hajime Morrita 2013-01-07 23:55:57 PST
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.
Comment 19 Yuki Sekiguchi 2013-01-08 00:01:48 PST
Thank you for your advice!
I try it at next time that commit queue is rejected.
Comment 20 WebKit Review Bot 2013-01-08 00:14:10 PST
Comment on attachment 181617 [details]
Patch

Clearing flags on attachment: 181617

Committed r139040: <http://trac.webkit.org/changeset/139040>
Comment 21 WebKit Review Bot 2013-01-08 00:14:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 noel gordon 2013-01-10 04:10:27 PST
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?
Comment 23 noel gordon 2013-01-10 04:19:25 PST
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
...
Comment 24 Yuki Sekiguchi 2013-01-17 20:58:40 PST
(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.