Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(21)

Issue 1986563002: Apply vertical-align style of <sub> and <sup> to child elements. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by joone
Modified:
1 year, 4 months ago
Reviewers:
yosin_UTC9
CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Apply vertical-align style of <sub> and <sup> to child elements. If the selected element has <sub> or <sup> ancestor element, apply the corresponding style(vertical-align) to it so that document.queryCommandState() works fine on the selected element. <sub> and <sup> tags are represented with CSS vertical-align property but they are not inherited. Therefore, we need to apply the property to child elements because <sub> and <sup> tag can have child elements such as <i> or <b> tag. BUG=582225 TEST=editing/execCommand/queryCommandState-04.html editing/execCommand/queryCommandState-04-mac.html Committed: https://crrev.com/dfe3d34820670521568ca7dfe681f701da94325b Cr-Commit-Position: refs/heads/master@{#396763}

Patch Set 1 #

Patch Set 2 : Update the test case #

Total comments: 6

Patch Set 3 : use range-based for loop and inline initializer #

Patch Set 4 : rename ancestorStyle to runnerStyle #

Patch Set 5 : fix the test failure on Mac #

Patch Set 6 : fix typos #

Patch Set 7 : add test cases for Mac #

Total comments: 1

Patch Set 8 : merge two test files #

Total comments: 1

Patch Set 9 : simplify the test case #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -4 lines) Patch
A third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html View 1 2 3 4 5 6 7 8 1 chunk +36 lines, -0 lines 1 comment Download
M third_party/WebKit/Source/core/editing/EditingStyle.h View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/EditingStyle.cpp View 1 2 3 4 5 8 chunks +35 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/editing/Editor.cpp View 1 2 3 4 5 1 chunk +4 lines, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 37 (13 generated)
joone
Hi, yosin@ could you review this CL?
1 year, 5 months ago (2016-05-17 00:26:23 UTC) #2
yosin_UTC9
The approach seems good. Nits for coding style. https://codereview.chromium.org/1986563002/diff/20001/third_party/WebKit/Source/core/editing/EditingStyle.cpp File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right): https://codereview.chromium.org/1986563002/diff/20001/third_party/WebKit/Source/core/editing/EditingStyle.cpp#newcode396 third_party/WebKit/Source/core/editing/EditingStyle.cpp:396: if ...
1 year, 5 months ago (2016-05-17 01:15:48 UTC) #3
joone
https://codereview.chromium.org/1986563002/diff/20001/third_party/WebKit/Source/core/editing/EditingStyle.cpp File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right): https://codereview.chromium.org/1986563002/diff/20001/third_party/WebKit/Source/core/editing/EditingStyle.cpp#newcode396 third_party/WebKit/Source/core/editing/EditingStyle.cpp:396: if (propertyID == CSSPropertyVerticalAlign) { On 2016/05/17 01:15:48, Yosi_UTC9 ...
1 year, 5 months ago (2016-05-17 23:01:33 UTC) #4
joone
Can I create a separated CL for Mac? Mac is different from other platforms when ...
1 year, 5 months ago (2016-05-18 01:10:38 UTC) #5
yosin_UTC9
On 2016/05/18 at 01:10:38, joone.hur wrote: > Can I create a separated CL for Mac? ...
1 year, 5 months ago (2016-05-18 02:20:00 UTC) #6
joone
On 2016/05/18 02:20:00, Yosi_UTC9 wrote: > On 2016/05/18 at 01:10:38, joone.hur wrote: > > Can ...
1 year, 5 months ago (2016-05-18 05:30:20 UTC) #7
yosin_UTC9
On 2016/05/18 at 05:30:20, joone.hur wrote: > On 2016/05/18 02:20:00, Yosi_UTC9 wrote: > > On ...
1 year, 5 months ago (2016-05-18 07:09:22 UTC) #8
joone
On 2016/05/18 07:09:22, Yosi_UTC9 wrote: > > Since, document.queryCommandState() is available for all platform. It ...
1 year, 5 months ago (2016-05-20 00:54:55 UTC) #11
yosin_UTC9
On 2016/05/20 at 00:54:55, joone.hur wrote: > On 2016/05/18 07:09:22, Yosi_UTC9 wrote: > > > ...
1 year, 4 months ago (2016-05-25 07:01:20 UTC) #15
joone
On 2016/05/25 07:01:20, Yosi_UTC9 wrote: > On 2016/05/20 at 00:54:55, joone.hur wrote: > > On ...
1 year, 4 months ago (2016-05-25 07:28:34 UTC) #16
yosin_UTC9
On 2016/05/25 at 07:28:34, joone.hur wrote: > On 2016/05/25 07:01:20, Yosi_UTC9 wrote: > > On ...
1 year, 4 months ago (2016-05-25 08:08:39 UTC) #17
joone
On 2016/05/25 08:08:39, Yosi_UTC9 wrote: > On 2016/05/25 at 07:28:34, joone.hur wrote: > > On ...
1 year, 4 months ago (2016-05-27 08:11:40 UTC) #20
yosin_UTC9
C++ changes are OK. We need to simplify test file. https://codereview.chromium.org/1986563002/diff/180001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04-mac.html File third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04-mac.html (right): https://codereview.chromium.org/1986563002/diff/180001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04-mac.html#newcode1 ...
1 year, 4 months ago (2016-05-30 01:28:12 UTC) #21
joone
On 2016/05/30 01:28:12, Yosi_UTC9 wrote: > C++ changes are OK. > We need to simplify ...
1 year, 4 months ago (2016-05-30 06:20:49 UTC) #23
yosin_UTC9
https://codereview.chromium.org/1986563002/diff/240001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html File third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html (right): https://codereview.chromium.org/1986563002/diff/240001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html#newcode33 third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html:33: if (window.internals) nit: How about this? Since, we don't ...
1 year, 4 months ago (2016-05-30 06:43:42 UTC) #25
joone
On 2016/05/30 06:43:42, Yosi_UTC9 wrote: > https://codereview.chromium.org/1986563002/diff/240001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html > File > third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html > (right): > > ...
1 year, 4 months ago (2016-05-30 07:30:30 UTC) #26
yosin_UTC9
On 2016/05/30 at 07:30:30, joone.hur wrote: > On 2016/05/30 06:43:42, Yosi_UTC9 wrote: > > https://codereview.chromium.org/1986563002/diff/240001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html ...
1 year, 4 months ago (2016-05-30 08:10:03 UTC) #27
joone
https://codereview.chromium.org/1986563002/diff/260001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html File third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html (right): https://codereview.chromium.org/1986563002/diff/260001/third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html#newcode35 third_party/WebKit/LayoutTests/editing/execCommand/queryCommandState-04.html:35: (window.internals ? ['mac', 'win'] : ['']).forEach(test_all); ==> (window.internals ? ...
1 year, 4 months ago (2016-05-30 09:16:18 UTC) #28
yosin_UTC9
lgtm
1 year, 4 months ago (2016-05-31 00:44:42 UTC) #29
yosin_UTC9
Thanks for patient with long iteration!
1 year, 4 months ago (2016-05-31 00:45:03 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986563002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986563002/260001
1 year, 4 months ago (2016-05-31 00:45:18 UTC) #32
commit-bot: I haz the power
Committed patchset #9 (id:260001)
1 year, 4 months ago (2016-05-31 00:48:35 UTC) #34
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/dfe3d34820670521568ca7dfe681f701da94325b Cr-Commit-Position: refs/heads/master@{#396763}
1 year, 4 months ago (2016-05-31 00:50:16 UTC) #36
joone
1 year, 4 months ago (2016-05-31 07:05:13 UTC) #37
Message was sent while issue was closed.
On 2016/05/31 00:45:03, Yosi_UTC9 wrote:
> Thanks for patient with long iteration!

The change I suggested in my last comment was not applied to the commit so I
created another pull request:
https://codereview.chromium.org/2020183002/

Thank you for the reviews!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa