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

Issue 231933004: Fix a broken test which calls Selection.setPosition(null). (Closed)

Created:
6 years, 8 months ago by yoichio
Modified:
6 years, 8 months ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Fix a broken test which calls Selection.setPosition(null). This test was copied from a sibling test: http://src.chromium.org/viewvc/blink/trunk/LayoutTests/editing/execCommand/indent-images-2.html This test calls document.getElementsByTagName('div') but there are no div elements. As a result, test calls Selection.setPosition(null) and it is clearly not intended. This test uses a span element instead of div element which is targeted in the indent-images-2.html. Thus this CL rewrite 'div' to 'span'. This test originally expects that execCommand('indent') should not indent contents inside inline block. However fixed case, which works as intended, indents the contents. This CL does only fix broken scripts. We already started discussion about the execCommand behavior. If we should change the implementation, we will do it on another patch. BUG=361911 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171745

Patch Set 1 #

Total comments: 2

Patch Set 2 : Modify typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -5 lines) Patch
M LayoutTests/editing/execCommand/indent-images-3.html View 1 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/editing/execCommand/indent-images-3-expected.txt View 1 1 chunk +6 lines, -3 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
yoichio
6 years, 8 months ago (2014-04-10 05:40:39 UTC) #1
Yuta Kitamura
I understand the original test was broken, but I don't think we could land this ...
6 years, 8 months ago (2014-04-10 06:00:17 UTC) #2
yoichio
On 2014/04/10 06:00:17, Yuta Kitamura wrote: > I understand the original test was broken, but ...
6 years, 8 months ago (2014-04-10 09:35:15 UTC) #3
Yuta Kitamura
On 2014/04/10 09:35:15, yoichio wrote: > IE10 works same as ours. > FireFox looks not ...
6 years, 8 months ago (2014-04-10 10:15:51 UTC) #4
yoichio
Currently we are discussing which is better implementation. But I want to split the execCommand ...
6 years, 8 months ago (2014-04-14 07:26:53 UTC) #5
Yuta Kitamura
The only concern I have is that <blockquote> inside <span> is invalid according to HTML ...
6 years, 8 months ago (2014-04-14 07:43:57 UTC) #6
yoichio
Rewrite the description.
6 years, 8 months ago (2014-04-16 01:30:11 UTC) #7
Yuta Kitamura
The change LGTM; want a consensus before landing whether we are going to tolerate <blockquote> ...
6 years, 8 months ago (2014-04-16 01:52:26 UTC) #8
yosin_UTC9
On 2014/04/16 01:52:26, Yuta Kitamura wrote: > The change LGTM; want a consensus before landing ...
6 years, 8 months ago (2014-04-16 02:30:36 UTC) #9
yoichio
Thanks. https://codereview.chromium.org/231933004/diff/1/LayoutTests/editing/execCommand/indent-images-3.html File LayoutTests/editing/execCommand/indent-images-3.html (right): https://codereview.chromium.org/231933004/diff/1/LayoutTests/editing/execCommand/indent-images-3.html#newcode17 LayoutTests/editing/execCommand/indent-images-3.html:17: + 'Indentation should success even if the root ...
6 years, 8 months ago (2014-04-16 03:17:02 UTC) #10
yoichio
The CQ bit was checked by yoichio@chromium.org
6 years, 8 months ago (2014-04-16 03:17:06 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/231933004/20001
6 years, 8 months ago (2014-04-16 03:17:30 UTC) #12
commit-bot: I haz the power
6 years, 8 months ago (2014-04-16 04:57:49 UTC) #13
Message was sent while issue was closed.
Change committed as 171745

Powered by Google App Engine
This is Rietveld 408576698