Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(10)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1173853005: LayoutFieldSet::computePreferredLogicalWidths uses marginLeft for marginRight by mistake

Created:
5 years, 6 months ago by a.berwal
Modified:
5 years, 5 months ago
Reviewers:
a.suchit2, a.suchit, esprehn
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

LayoutFieldSet::computePreferredLogicalWidths uses marginLeft for marginRight by mistake Changing legendMarginRight = legend->style()->marginLeft() to legendMarginRight = legend->style()->marginRight() BUG=498532

Patch Set 1 #

Patch Set 2 : Adding Layout Tests after removing the functions #

Total comments: 1

Patch Set 3 : Adding layout Test #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -2 lines) Patch
A LayoutTests/fast/block/basic/fieldset-with-legend-right-margin.html View 1 2 1 chunk +32 lines, -0 lines 3 comments Download
A LayoutTests/fast/block/basic/fieldset-with-legend-right-margin-expected.txt View 1 2 1 chunk +12 lines, -0 lines 0 comments Download
M Source/core/layout/LayoutFieldset.cpp View 1 2 2 chunks +1 line, -2 lines 0 comments Download

Messages

Total messages: 10 (2 generated)
a.suchit2
lgtm
5 years, 6 months ago (2015-06-10 08:09:42 UTC) #2
a.berwal
PTAL
5 years, 6 months ago (2015-06-10 11:55:09 UTC) #4
esprehn
This needs a test.
5 years, 6 months ago (2015-06-12 06:18:44 UTC) #5
a.suchit2
PR NLGTM https://chromiumcodereview.appspot.com/1173853005/diff/20001/Source/core/layout/LayoutFieldset.h File Source/core/layout/LayoutFieldset.h (left): https://chromiumcodereview.appspot.com/1173853005/diff/20001/Source/core/layout/LayoutFieldset.h#oldcode45 Source/core/layout/LayoutFieldset.h:45: virtual void computePreferredLogicalWidths() override; Do not remove ...
5 years, 5 months ago (2015-07-09 10:09:55 UTC) #6
a.suchit2
use check Layout test with "data-expected-margin-right". You can refer : https://chromiumcodereview.appspot.com/821203003/diff/140001/LayoutTests/fast/table/table-rowspan-height-less-than-one-percent.html
5 years, 5 months ago (2015-07-09 10:34:19 UTC) #7
a.berwal
On 2015/07/09 10:34:19, a.suchit2 wrote: > use check Layout test with "data-expected-margin-right". > > You ...
5 years, 5 months ago (2015-07-13 04:59:57 UTC) #8
esprehn
I tried this in Canary and it also prints PASS 4 times, you need a ...
5 years, 5 months ago (2015-07-13 06:02:43 UTC) #9
a.suchit
5 years, 5 months ago (2015-07-13 12:59:10 UTC) #10 
Test case have too many style property used. If you make is simplest test case
using less css properties would be better.

Try not to use css properties with html tags so test case looks clean and easily
understandable.

https://codereview.chromium.org/1173853005/diff/40001/LayoutTests/fast/block/...
File LayoutTests/fast/block/basic/fieldset-with-legend-right-margin.html
(right):

https://codereview.chromium.org/1173853005/diff/40001/LayoutTests/fast/block/...
LayoutTests/fast/block/basic/fieldset-with-legend-right-margin.html:9: <fieldset
data-expected-margin-right="10" style="position:
absolute;margin-left:20px;margin-right:10px;">
we are correcting the legend margin right so data-expected-margin-right should
be set for legend.

Powered by Google App Engine
This is Rietveld 408576698