Bug 156900 – Add WKPreference for SelectionPaintingWithoutSelectionGaps

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 277029 [details]
Patch

[Gyuyoung Kim]

Created attachment 277033 [details]
Enable selection gap painting

[Gyuyoung Kim]

Created attachment 277034 [details]
Disable selection gap painting

[Michael Catanzaro]

Before/after screenshots would be interesting

[Gyuyoung Kim]

(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 ?

[Michael Catanzaro]

(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.

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

@darin, @dhyatt, any comment on this ?

[Michael Catanzaro]

(In reply to comment #4)
> Before/after screenshots would be interesting

I'd really like to see what the selection gap is. :)

[Gyuyoung Kim]

Created attachment 277480 [details]
Enable selection gap painting

[Gyuyoung Kim]

Created attachment 277481 [details]
Disable selection gap painting

[Gyuyoung Kim]

(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.

[Michael Catanzaro]

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.

[Darin Adler]

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.

[Antonio Gomes]

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.

[Darin Adler]

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.

[Michael Catanzaro]

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.

[Gyuyoung Kim]

(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.

[Michael Catanzaro]

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.)

[Gyuyoung Kim]

Created attachment 277976 [details]
Patch

[Gyuyoung Kim]

(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.

[WebKit Commit Bot]

Comment on attachment 277976 [details]
Patch

Clearing flags on attachment: 277976

Committed r200412: <http://trac.webkit.org/changeset/200412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.