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 EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dgatwood, enrica, eric, hyatt, jparent, michaelthomas, morrita, ojan, rniwa, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
test
none
patch v0
none
v1; made the test case self-descriptive ojan: review+

Description Ojan Vafai 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.
Comment 1 Tony Chang 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'.
Comment 2 Hajime Morrita 2010-03-07 23:30:43 PST
Created attachment 50191 [details]
test

I've attached a repro.
Comment 3 Hajime Morrita 2010-03-08 04:35:54 PST
Created attachment 50210 [details]
patch v0
Comment 4 Ojan Vafai 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.
Comment 5 Ojan Vafai 2010-03-25 11:47:19 PDT
+hyatt who originally wrote this code.
Comment 6 Hajime Morrita 2010-03-26 03:18:18 PDT
Created attachment 51722 [details]
v1; made the test case self-descriptive
Comment 7 Hajime Morrita 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.
Comment 8 Adam Barth 2010-05-13 00:25:46 PDT
More ancient  edit patches.  @enrica or @ojan?
Comment 9 Dave Hyatt 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.
Comment 10 Ojan Vafai 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
Comment 11 Hajime Morrita 2010-05-28 00:02:10 PDT
Committed r60344: <http://trac.webkit.org/changeset/60344>
Comment 12 David Gatwood 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.