Bug 156900

Summary: Add WKPreference for SelectionPaintingWithoutSelectionGaps
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: New BugsAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, hyatt, kondapallykalyan, mcatanzaro, simon.fraser, tonikitoo
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Enable selection gap painting
none
Disable selection gap painting
none
Enable selection gap painting
none
Disable selection gap painting
none
Patch none

Description Gyuyoung Kim 2016-04-22 02:18:42 PDT
Unlike other browsers WebKit has been drawing selection gaps between render blocks during the text selection.
This often cause text selection screen looks messy. This patch adds a setting method to disable the functionality,
and EFL port disables it by default.
Comment 1 Gyuyoung Kim 2016-04-22 02:21:16 PDT
Created attachment 277029 [details]
Patch
Comment 2 Gyuyoung Kim 2016-04-22 02:31:12 PDT
Created attachment 277033 [details]
Enable selection gap painting
Comment 3 Gyuyoung Kim 2016-04-22 02:31:47 PDT
Created attachment 277034 [details]
Disable selection gap painting
Comment 4 Michael Catanzaro 2016-04-23 18:52:29 PDT
Before/after screenshots would be interesting
Comment 5 Gyuyoung Kim 2016-04-24 18:12:17 PDT
(In reply to comment #4)
> Before/after screenshots would be interesting

Browsers based on WebKit only draw gap areas between selection blocks. I think some browsers definitely want to disable this option. Hyatt, could you review this patch ?
Comment 6 Michael Catanzaro 2016-04-25 07:16:23 PDT
(In reply to comment #5)
> Browsers based on WebKit only draw gap areas between selection blocks.

I don't know what this means... when adding new settings, we need a strong argument why some browsers need the setting on and others need it off.
Comment 7 Gyuyoung Kim 2016-04-25 17:38:35 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Browsers based on WebKit only draw gap areas between selection blocks.
> 
> I don't know what this means... when adding new settings, we need a strong
> argument why some browsers need the setting on and others need it off.

Because I don't know what port still wants to use this functionality. That's why I add a new settings option to enable/disable the function. At least EFL port wants to disable the selection gap painting.
Comment 8 Gyuyoung Kim 2016-04-26 19:23:16 PDT
@darin, @dhyatt, any comment on this ?
Comment 9 Michael Catanzaro 2016-04-27 07:15:21 PDT
(In reply to comment #4)
> Before/after screenshots would be interesting

I'd really like to see what the selection gap is. :)
Comment 10 Gyuyoung Kim 2016-04-27 08:32:29 PDT
Created attachment 277480 [details]
Enable selection gap painting
Comment 11 Gyuyoung Kim 2016-04-27 08:33:13 PDT
Created attachment 277481 [details]
Disable selection gap painting
Comment 12 Gyuyoung Kim 2016-04-27 08:38:56 PDT
(In reply to comment #9)
> (In reply to comment #4)
> > Before/after screenshots would be interesting
> 
> I'd really like to see what the selection gap is. :)

To show what is different between on/off the selection gap painting, I capture screenshot on WebKitGTK :)

Besides I mark some areas of selection gap using red circles. When the selection gap painting is disabled, I think we're able to recognize what content blocks are selected more clear.
Comment 13 Michael Catanzaro 2016-04-27 10:00:20 PDT
Ah OK, I missed the first screenshots you attached, sorry. I also prefer to not paint selection gaps; we'll probably do that by default for GTK as well.

Unless any port really wants the current behavior, maybe we can avoid adding the setting.
Comment 14 Darin Adler 2016-04-27 11:16:42 PDT
The OS X port definitely wants the current behavior, or ideally a much improved version of it that doesn’t draw overlapping areas of selection color. Especially for clients like the Mail app. This is an important part of the how selections look on that platform across multiple apps.

The version where the selection color is drawn only under text is much easier to implement, of course.
Comment 15 Antonio Gomes 2016-04-27 11:23:52 PDT
This could probably tested by something like this:

1) do a selection with "GAP selection" preference on.
2) do a mouse down + move on the "GAP selection" area
3) It should trigger drag'n drop.

On the other hand, add a test with "GAP selection" OFF.

1) do a selection with "GAP selection" off.
2) do a mouse down + move at the same spot "GAP selection" would be present, if preference on ON
3) It should *not* trigger drag'n drop.

Also, Michael's point is interesting. Does any port actually want "GAP selection" enabled by default at this point.

I imagine this has been ON by default for Safari, but what do Firefox/Chrome on Mac OS X do?
Maybe Safari people wants to match other vendors here.

In any case, you would get broader audience if you mailed webkit-dev about it.
Comment 16 Darin Adler 2016-04-27 11:26:20 PDT
Other web browsers tend to make their selections look like selections do on Windows, for historical reasons. That’s probably not what is wanted for Safari.

The WebKit team worked hard to implement this so that text editing in apps like Mail and in text areas inside Safari would look the way it’s supposed to on Mac. On the other hand, it’s not as obvious how to generalize this for complex webpage layouts. The more simplistic idea of how to draw selections that is done on Windows and mentioned in the CSS specification is easier to understand and I think it’s what all the other browser engines do.
Comment 17 Michael Catanzaro 2016-04-27 11:37:43 PDT
Sounds like this needs to be a preference, then.

(In reply to comment #15)
> Also, Michael's point is interesting. Does any port actually want "GAP
> selection" enabled by default at this point.

I'm not really sure what we want for GTK after all. Playing around with some other apps like gedit and Terminal, it seems painting gap selection is expected on our platform, too.
Comment 18 Gyuyoung Kim 2016-05-02 18:08:24 PDT
(In reply to comment #16)
> Other web browsers tend to make their selections look like selections do on
> Windows, for historical reasons. That’s probably not what is wanted for
> Safari.
> 
> The WebKit team worked hard to implement this so that text editing in apps
> like Mail and in text areas inside Safari would look the way it’s supposed
> to on Mac. On the other hand, it’s not as obvious how to generalize this for
> complex webpage layouts. The more simplistic idea of how to draw selections
> that is done on Windows and mentioned in the CSS specification is easier to
> understand and I think it’s what all the other browser engines do.

It seems to me that other ports still want to use existing selection painting with the selection gap, right ? But Chrome and Firefox don't support the selection gap painting unlike WebKit. So I think this new option can give us choice to use or not it. I'm going to send a mail to webkit-dev about this new option.
Comment 19 Michael Catanzaro 2016-05-02 18:40:33 PDT
Comment on attachment 277029 [details]
Patch

I think it's probably uncontroversial, r=me. Clearly different ports require different behavior, and a setting seems more appropriate for this than conditional compilation in WebCore. The only change to WebCore is a new return false in RenderBlock::shouldPaintSelectionGaps, which looks very probably safe. And the changes in WebKit2 are just settings boilerplate. (It would be good for an owner to rubber-stamp the WebKit2 changes.)
Comment 20 Gyuyoung Kim 2016-05-02 21:27:25 PDT
Created attachment 277976 [details]
Patch
Comment 21 Gyuyoung Kim 2016-05-03 03:36:01 PDT
(In reply to comment #19)
> Comment on attachment 277029 [details]
> Patch
> 
> I think it's probably uncontroversial, r=me. Clearly different ports require
> different behavior, and a setting seems more appropriate for this than
> conditional compilation in WebCore. The only change to WebCore is a new
> return false in RenderBlock::shouldPaintSelectionGaps, which looks very
> probably safe. And the changes in WebKit2 are just settings boilerplate. (It
> would be good for an owner to rubber-stamp the WebKit2 changes.)

Thanks Michael, but I'm not sure if this patch can be landed, because non-WK2 owner reviewed. If there is no objections to land, I would like to set cq+ tomorrow.
Comment 22 WebKit Commit Bot 2016-05-04 01:38:31 PDT
Comment on attachment 277976 [details]
Patch

Clearing flags on attachment: 277976

Committed r200412: <http://trac.webkit.org/changeset/200412>
Comment 23 WebKit Commit Bot 2016-05-04 01:38:37 PDT
All reviewed patches have been landed.  Closing bug.