Bug 129803

Summary:

text selection highlight is visually incorrect under heavy padding constraints

Product:

WebKit

Reporter:

Mario Sanchez Prada <mario>

Component:

Text

Assignee:

Nobody <webkit-unassigned>

Status:

NEW ---

Severity:

Normal

CC:

buildbot, commit-queue, esprehn+autocc, glenn, hyatt, kling, kondapallykalyan, mitz, rniwa, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch proposal	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion	none
Patch proposal	rniwa: review-

Description Mario Sanchez Prada 2014-03-06 08:05:02 PST

This bug is tracks down an issue as reported for Blink as issue 313593[1]. Description of the issue as reported there is as follows:

Steps to reproduce the problem:
1. Load the following HTML in WebKit:
<html>
<body>
<div style="font-size: 16px; height: 20px; line-height: 0px;">
  The House
</div>
<div style="font-size: 16px; height: 20px; line-height: 32px;">
  The House
</div>
</body>
</html>

2. Select all the text in each line separately (one line at a time)


What is the expected behavior?
You should see the selection rectangle properly drawn (not too small, not too big) when selecting "The House" in any of the two lines


What went wrong?
You see something like this for the two lines:
 - 1st line: http://goo.gl/4rgBCV
 - 2nd line: http://goo.gl/UToFkb


[1] https://code.google.com/p/chromium/issues/detail?id=313593

Comment 1 Mario Sanchez Prada 2014-03-06 08:49:29 PST

Created attachment 225994 [details]
Patch proposal

Patch proposal backporting the fix (and tests) from blink revision r167813

Comment 2 Build Bot 2014-03-06 09:55:59 PST

Comment on attachment 225994 [details]
Patch proposal

Attachment 225994 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4607503778185216

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

Comment 3 Build Bot 2014-03-06 09:56:02 PST

Created attachment 226003 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

Comment 4 Build Bot 2014-03-06 10:10:01 PST

Comment on attachment 225994 [details]
Patch proposal

Attachment 225994 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6134646777577472

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

Comment 5 Build Bot 2014-03-06 10:10:05 PST

Created attachment 226006 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Comment 6 Build Bot 2014-03-06 11:10:01 PST

Comment on attachment 225994 [details]
Patch proposal

Attachment 225994 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6702313141960704

New failing tests:
fast/text/selection-rect-line-height-too-big.html
editing/selection/inline-closest-leaf-child.html
fast/text/selection-rect-line-height-too-small.html
fast/inline-block/14498-positionForCoordinates.html

Comment 7 Build Bot 2014-03-06 11:10:04 PST

Created attachment 226013 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Comment 8 Mario Sanchez Prada 2014-03-07 02:11:59 PST

Created attachment 226101 [details]
Patch proposal

Considered output from EWS mac bots

Comment 9 Mario Sanchez Prada 2014-03-13 08:58:48 PDT

Ping reviewers?

Comment 10 Ryosuke Niwa 2014-03-13 14:49:19 PDT

Comment on attachment 226101 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=226101&action=review

> LayoutTests/platform/mac/editing/selection/inline-closest-leaf-child-expected.txt:35
> -selection start: position 2 of child 0 {#text} of child 1 {SPAN} of child 4 {DIV} of body
> -selection end:   position 5 of child 2 {#text} of child 1 {SPAN} of child 4 {DIV} of body
> +caret: position 0 of child 0 {#text} of child 1 {SPAN} of child 4 {DIV} of body

Looks like this is a regression. r- due to the lack of description as to why new result is correct.

Comment 11 Mario Sanchez Prada 2014-03-14 06:34:12 PDT

(In reply to comment #10)
> (From update of attachment 226101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226101&action=review
> 
> > LayoutTests/platform/mac/editing/selection/inline-closest-leaf-child-expected.txt:35
> > -selection start: position 2 of child 0 {#text} of child 1 {SPAN} of child 4 {DIV} of body
> > -selection end:   position 5 of child 2 {#text} of child 1 {SPAN} of child 4 {DIV} of body
> > +caret: position 0 of child 0 {#text} of child 1 {SPAN} of child 4 {DIV} of body
> 
> Looks like this is a regression. r- due to the lack of description as to why new result is correct.

You are right, sorry for not noticing it while uploading the patch.

However, I think that such a regression is not actually caused by this patch, which seems to be unveiling the issue instead. I say this because I've manually loaded and experimented with the same test in Firefox and Blink (through a recently built version of chromium which includes this patch) and in both cases the behaviour is the expected one: I can select individual characters from "ipsum" by dragging the mouse in the yellow area below.

However, if I try to do the same thing in WebKit, I get different results depending on whether this patch is applied or not (but both seem to be wrong):

* Without the patch: I'm able to select characters by dragging in the yellow area above "ipsum", but the blue selection rectangle drawn is incorrect (height is too much, invading the yellow area).

* With the patch: I can't select anything at all by dragging in the yellow area above "ipsum", which is also wrong. This does not happen in Blink with the same patch applied, though.

As mentioned before, this backport patch addresses the first issue (selection rectangle incorrectly drawn) above. Unfortunately, it also unveils the second issue, which I believe is not related to this change since that's not happening in Blink after the very same changeset has been incorporated (I can select dragging in the yellow area && the selection rectangle is properly drawn, as in FF).

So, perhaps the right thing to do here would be, instead of rebaselining those expectations (which I agree is wrong), to file a bug for the selection while dragging in the yellow area and add that test to the TestExpectations files.

What do you think?

Comment 12 Mario Sanchez Prada 2014-03-24 03:30:53 PDT

(In reply to comment #11)
> [...]
> So, perhaps the right thing to do here would be, instead of rebaselining those 
> expectations (which I agree is wrong), to file a bug for the selection while 
> dragging in the yellow area and add that test to the TestExpectations files.
> 
> What do you think?

It's been a while already since this comment. "Kindly" pinging then :)