Bug 32123

Summary:

cursor movement and text selection does not work well if a block is followed by an inline

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dgatwood, enrica, eric, hyatt, jparent, michaelthomas, morrita, ojan, rniwa, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
test	none
patch v0	none
v1; made the test case self-descriptive	ojan: review+

Ojan Vafai

Reported 2009-12-03 10:14:10 PST

Moving this over from https://bugs.webkit.org/show_bug.cgi?id=26635#c3 - Go to the midas demo: http://www.mozilla.org/editor/midasdemo - Enter the following HTML: <div>a</div><span>b</span> - Place the cursor before 'a'. - Hold down shift and press the down arrow. - Note that the entire contentEditable element has been selected.

Attachments
test (295 bytes, text/html) 2010-03-07 23:30 PST, Hajime Morrita	no flags	Details
patch v0 (14.40 KB, patch) 2010-03-08 04:35 PST, Hajime Morrita	no flags	Details Formatted Diff Diff
v1; made the test case self-descriptive (22.27 KB, patch) 2010-03-26 03:18 PDT, Hajime Morrita	ojan: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Tony Chang

Comment 1 2010-02-21 20:57:46 PST

(In reply to comment #0) > Moving this over from https://bugs.webkit.org/show_bug.cgi?id=26635#c3 > > - Go to the midas demo: http://www.mozilla.org/editor/midasdemo > - Enter the following HTML: > <div>a</div><span>b</span> > - Place the cursor before 'a'. > - Hold down shift and press the down arrow. > - Note that the entire contentEditable element has been selected. It looks like this is only a bug in how the selection highlight is painted. Even though the whole area appears selected, pressing delete only deletes the 'a'.

Hajime Morrita

Comment 2 2010-03-07 23:30:43 PST

Created attachment 50191 [details] test I've attached a repro.

Hajime Morrita

Comment 3 2010-03-08 04:35:54 PST

Created attachment 50210 [details] patch v0

Ojan Vafai

Comment 4 2010-03-25 11:42:47 PDT

Comment on attachment 50210 [details] patch v0 > +++ b/LayoutTests/editing/selection/range-between-block-and-inline.html > @@ -0,0 +1,17 @@ > +<html> > +<head> > +<script> > +function doTest() > +{ Maybe add a comment that this is testing a selection paint bug and so it needs to be a pixel test? > + var range = document.createRange(); > + range.selectNode(document.getElementById("target")); > + document.getSelection().addRange(range); > +} The code changes look correct to me.

Ojan Vafai

Comment 5 2010-03-25 11:47:19 PDT

+hyatt who originally wrote this code.

Hajime Morrita

Comment 6 2010-03-26 03:18:18 PDT

Created attachment 51722 [details] v1; made the test case self-descriptive

Hajime Morrita

Comment 7 2010-03-26 03:22:16 PDT

Ojan, thank you for reviewing. I updated the patch. > Maybe add a comment that this is testing a selection paint bug and so it needs > to be a pixel test? Added the comment to describe what expected. Unfortunately, this test case need a pixel test because the bug sits on the painting, not on the layout. So we cannot sense its internal state with DOM nor dumps.

Adam Barth

Comment 8 2010-05-13 00:25:46 PDT

More ancient edit patches. @enrica or @ojan?

Dave Hyatt

Comment 9 2010-05-27 14:41:00 PDT

Comment on attachment 51722 [details] v1; made the test case self-descriptive In terms of the visual rendering of selection, what you're supposed to have is a bunch of leaf elements, which are replaced elements and text, the line boxes, and then the containing blocks for those elements, which are responsible for line/block gap filling. RenderInlines shouldn't really be examined at all for selection state, and they certainly shouldn't have it set on themselves. We should add ASSERTs when RenderInline's selection state is set and see why this happened. I'm guessing maybe it got set because it was m_selectionStart or m_selectionEnd, but it would be good to know. If we want RenderInline selectionState to be accurate, then everyone would need to start propagating selection state up through inlines. That could seriously slow down selection performance. If you look at all of the selectionState methods, they all skip intermediate inlines and jump right up to the RenderBlock. It might be ok for RenderInline to have questionable accuracy but still propagate, but that seems kind of weird to me. Maybe it would be ok though. I'd really like to understand why this happened in the first place though.

Ojan Vafai

Comment 10 2010-05-27 14:55:46 PDT

Comment on attachment 51722 [details] v1; made the test case self-descriptive Please add the FIXME before propagating up to the containingBlock in RenderBoxModelObject. From #webkit: ojan: dhyatt: added the assert. the end of the selection is a RenderInline dhyatt: yeah unsurprising dhyatt: i guess the patch is ok... let's just put a FIXME in asking whether we should consider propagating to ancestor RenderInlines dhyatt: but for now we can just leave everything jumping out to the containing block dhyatt: i suppose you could say selection consists of the start, end, leaves, and their containing blocks dhyatt: where the start/end violate the leaf rule

Hajime Morrita

Comment 11 2010-05-28 00:02:10 PDT

Committed r60344: <http://trac.webkit.org/changeset/60344>

David Gatwood

Comment 12 2014-12-15 15:28:44 PST

FYI, this fix is wrong, and part of the fix breaks highlighting for ::before and ::after pseudo-elements once you enable it. However, with my fix for that bug, this issue does not reproduce even after removing the relevant portion of this fix (the chunk of code that starts with "FIXME: We should consider whether it is OK propagating to ancestor RenderInlines."). I'll add more details in bug 15256, where I'm about to submit a preliminary fix for the pseudo-element highlighting issue.

Note You need to log in before you can comment on or make changes to this bug.