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-

Mario Sanchez Prada

Reported 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

Attachments
Patch proposal (60.41 KB, patch) 2014-03-06 08:49 PST, Mario Sanchez Prada	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (578.26 KB, application/zip) 2014-03-06 09:56 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (604.92 KB, application/zip) 2014-03-06 10:10 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (605.84 KB, application/zip) 2014-03-06 11:10 PST, Build Bot	no flags	Details
Patch proposal (64.01 KB, patch) 2014-03-07 02:11 PST, Mario Sanchez Prada	rniwa: review-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Mario Sanchez Prada

Comment 1 2014-03-06 08:49:29 PST

Created attachment 225994 [details] Patch proposal Patch proposal backporting the fix (and tests) from blink revision r167813

Build Bot

Comment 2 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

Build Bot

Comment 3 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

Build Bot

Comment 4 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

Build Bot

Comment 5 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

Build Bot

Comment 6 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

Build Bot

Comment 7 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

Mario Sanchez Prada

Comment 8 2014-03-07 02:11:59 PST

Created attachment 226101 [details] Patch proposal Considered output from EWS mac bots

Mario Sanchez Prada

Comment 9 2014-03-13 08:58:48 PDT

Ping reviewers?

Ryosuke Niwa

Comment 10 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.

Mario Sanchez Prada

Comment 11 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?

Mario Sanchez Prada

Comment 12 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 :)

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