Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023433002:
Use 1em margin for list-style:inside items (Closed)
|
Created:
4 years, 6 months ago by Gleb Lanbin Modified:
4 years, 6 months ago Reviewers:
eae CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-css, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, dglazkov+blink, apavlov+blink_chromium.org, jchaffraix+rendering, darktears, blink-reviews, blink-reviews-layout_chromium.org, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix the interop issue for list-style:inside items. W3C spec http://www.w3.org/TR/css3-lists/#ua-stylesheet recommends 1em margin for markers. Unlike other browsers (verified with Edge and Firefox) Chrome does not follow UA stylesheet recommendation.
The fix is temporarily landed in LayoutListMarker.cpp. Default UA CSS should be moved to WebKit/Source/core/css/html.css after crbug.com/457718 is fixed.
BUG=547938
Committed: https://crrev.com/3feed678d176446eb08b6c1df16a9f1b3d0666ac
Cr-Commit-Position: refs/heads/master@{#397582}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Fix fast/events/onclick-list-marker.html test #
Messages
Total messages: 51 (30 generated)
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Fix 1em margin for list items BUG=547938 ========== to ========== BUG=547938 ==========
Description was changed from ========== BUG=547938 ========== to ========== Fix the interop issue for list-style:inside items. W3C spec http://www.w3.org/TR/css3-lists/#ua-stylesheet recommends 1em margin for markers. Unlike other browsers (verified with Edge and Firefox) Chrome does not follow UA stylesheet recommendation. The fix is temporarily landed in LayoutListMarker.cpp. Default UA CSS should be moved to WebKit/Source/core/css/html.css after crbug.com/457718 is fixed. BUG=547938 ==========
Patchset #1 (id:1) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:160001) has been deleted
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
glebl@chromium.org changed reviewers: + eae@chromium.org
Thanks for doing this! https://codereview.chromium.org/2023433002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right): https://codereview.chromium.org/2023433002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:42: const int cUAMarkerMargin = 1; Please include the unit type in the name. cUAMarkerMarginEm https://codereview.chromium.org/2023433002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:269: marginEnd = fontMetrics.ascent() - minPreferredLogicalWidth() + 1 + cUAMarkerMargin; This adds the margin as a pixel value, does it not?
https://codereview.chromium.org/2023433002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right): https://codereview.chromium.org/2023433002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:42: const int cUAMarkerMargin = 1; On 2016/05/31 22:37:08, eae wrote: > Please include the unit type in the name. > > cUAMarkerMarginEm Done. https://codereview.chromium.org/2023433002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:269: marginEnd = fontMetrics.ascent() - minPreferredLogicalWidth() + 1 + cUAMarkerMargin; On 2016/05/31 22:37:08, eae wrote: > This adds the margin as a pixel value, does it not? Done.
https://codereview.chromium.org/2023433002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right): https://codereview.chromium.org/2023433002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:269: marginEnd = fontMetrics.ascent() - minPreferredLogicalWidth() + 1 + LayoutUnit(cUAMarkerMarginEm); It's a little more complicated than this I'm afraid You'll need to multiply the number of ems (cUAMarkerMarginEm) by the effective font size and then convert it to a LayoutUnit. Something like the following: LayoutUnit(cUAMarkerMarginEm * style()->computedFontSize())
https://codereview.chromium.org/2023433002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutListMarker.cpp (right): https://codereview.chromium.org/2023433002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutListMarker.cpp:269: marginEnd = fontMetrics.ascent() - minPreferredLogicalWidth() + 1 + LayoutUnit(cUAMarkerMarginEm); On 2016/05/31 23:42:27, eae wrote: > It's a little more complicated than this I'm afraid > > You'll need to multiply the number of ems (cUAMarkerMarginEm) by the effective > font size and then convert it to a LayoutUnit. > > Something like the following: > > LayoutUnit(cUAMarkerMarginEm * style()->computedFontSize()) Done. I wrongly assumed that LayoutUnit uses EM units. thanks.
LGTM!
The CQ bit was checked by glebl@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2023433002/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2023433002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/2023433002/#ps280001 (title: "Fix fast/events/onclick-list-marker.html test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023433002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023433002/280001
Message was sent while issue was closed.
Description was changed from ========== Fix the interop issue for list-style:inside items. W3C spec http://www.w3.org/TR/css3-lists/#ua-stylesheet recommends 1em margin for markers. Unlike other browsers (verified with Edge and Firefox) Chrome does not follow UA stylesheet recommendation. The fix is temporarily landed in LayoutListMarker.cpp. Default UA CSS should be moved to WebKit/Source/core/css/html.css after crbug.com/457718 is fixed. BUG=547938 ========== to ========== Fix the interop issue for list-style:inside items. W3C spec http://www.w3.org/TR/css3-lists/#ua-stylesheet recommends 1em margin for markers. Unlike other browsers (verified with Edge and Firefox) Chrome does not follow UA stylesheet recommendation. The fix is temporarily landed in LayoutListMarker.cpp. Default UA CSS should be moved to WebKit/Source/core/css/html.css after crbug.com/457718 is fixed. BUG=547938 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Fix the interop issue for list-style:inside items. W3C spec http://www.w3.org/TR/css3-lists/#ua-stylesheet recommends 1em margin for markers. Unlike other browsers (verified with Edge and Firefox) Chrome does not follow UA stylesheet recommendation. The fix is temporarily landed in LayoutListMarker.cpp. Default UA CSS should be moved to WebKit/Source/core/css/html.css after crbug.com/457718 is fixed. BUG=547938 ========== to ========== Fix the interop issue for list-style:inside items. W3C spec http://www.w3.org/TR/css3-lists/#ua-stylesheet recommends 1em margin for markers. Unlike other browsers (verified with Edge and Firefox) Chrome does not follow UA stylesheet recommendation. The fix is temporarily landed in LayoutListMarker.cpp. Default UA CSS should be moved to WebKit/Source/core/css/html.css after crbug.com/457718 is fixed. BUG=547938 Committed: https://crrev.com/3feed678d176446eb08b6c1df16a9f1b3d0666ac Cr-Commit-Position: refs/heads/master@{#397582} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3feed678d176446eb08b6c1df16a9f1b3d0666ac Cr-Commit-Position: refs/heads/master@{#397582} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
