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

Issue 165933002: Fix the crash bug about selecting a marquee element. (Closed)

Created:
6 years, 10 months ago by yoichio
Modified:
6 years, 10 months ago
CC:
blink-reviews, dglazkov+blink, sof, eae+blinkwatch, adamk+blink_chromium.org, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Fix the crash bug about selecting a marquee element. This assertion is in CompositeEditCommand::moveParagraphs, L1223. The assertion error happens because VisibleSelection(destination, originalIsDirectional) returns default VisibleSelection value If desitination is a marquee element. Inside the constructor, VisiblePosition(position, _).deepPosition() returns default VisiblePosition value if position is a marquee element. Because VisiblePosition::canonicalPosition(position) returns default Position value if position is a marquee element. This is caused by that position.upstream() returns parent Node if position is a marquee element. This is wrong. When a marquee element is moving, its upstream should be itself. To fix this issue, this cl change endsOfNodeAreVisuallyDistinctPositions() called enclosingVisualBoundary() which is called from upstream(). This cl make endsOfNodeAreVisuallyDistinctPositions(node) return true if node is a marquee element. BUG=338424 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167617

Patch Set 1 #

Total comments: 8

Patch Set 2 : update #

Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, --1 lines) Patch
A LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html View 1 1 chunk +20 lines, -0 lines 0 comments Download
A + LayoutTests/editing/execCommand/selectAll-including-marquee-crash-expected.txt View 0 chunks +-1 lines, --1 lines 0 comments Download
M Source/core/dom/Position.cpp View 1 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
yoichio
6 years, 10 months ago (2014-02-14 04:42:59 UTC) #1
leviw_travelin_and_unemployed
Nearly there. A couple nits. https://codereview.chromium.org/165933002/diff/1/LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html File LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html (right): https://codereview.chromium.org/165933002/diff/1/LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html#newcode1 LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html:1: <style> Let's clean this ...
6 years, 10 months ago (2014-02-14 04:57:45 UTC) #2
leviw_travelin_and_unemployed
Also I'd add some more logic in your description. Tell us what was wrong and ...
6 years, 10 months ago (2014-02-14 05:18:38 UTC) #3
yoichio
https://codereview.chromium.org/165933002/diff/1/LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html File LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html (right): https://codereview.chromium.org/165933002/diff/1/LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html#newcode1 LayoutTests/editing/execCommand/selectAll-including-marquee-crash.html:1: <style> On 2014/02/14 04:57:46, Levi wrote: > Let's clean ...
6 years, 10 months ago (2014-02-19 08:00:30 UTC) #4
leviw_travelin_and_unemployed
>But marquee elements are moving so VisiblePosition of the node should not be same to ...
6 years, 10 months ago (2014-02-19 18:17:25 UTC) #5
yoichio
Update the CL description.
6 years, 10 months ago (2014-02-20 07:06:17 UTC) #6
leviw_travelin_and_unemployed
lgtm
6 years, 10 months ago (2014-02-21 22:06:53 UTC) #7
leviw_travelin_and_unemployed
The CQ bit was checked by leviw@chromium.org
6 years, 10 months ago (2014-02-21 22:06:58 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/165933002/80001
6 years, 10 months ago (2014-02-21 22:08:13 UTC) #9
commit-bot: I haz the power
6 years, 10 months ago (2014-02-21 23:44:44 UTC) #10
Message was sent while issue was closed.
Change committed as 167617

Powered by Google App Engine
This is Rietveld 408576698