RESOLVED FIXED Bug 32123
cursor movement and text selection does not work well if a block is followed by an inline 
https://bugs.webkit.org/show_bug.cgi?id=32123
Summary cursor movement and text selection does not work well if a block is followed ...
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
DetailsFormatted DiffDiff
v1; made the test case self-descriptive (22.27 KB, patch)
2010-03-26 03:18 PDT, Hajime Morrita 		ojan: review+
DetailsFormatted DiffDiff
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.