1479760 - Font Highlighter changes color when interacting with page

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P3)

Description

[Martin Balfanz [:mbalfanz]]

Attachment: font-highlighter.mp4

On initial use, the font highlighter highlights in grey.  After interacting with the page, it highlights in the specified selection color.

STR:
1. Visit any website
2. Inspect any text element and open the fonts panel
3. Hover over the font family name
4. Click anywhere on the page
5. Hover the font family name again

ER:
- the highlighter color should always be the selection color

AR:
- the highlighter color is grey in step 3, then changes in step 5 to the selection color (either specified by the system or CSS)

Comment 1

[Martin Balfanz [:mbalfanz]]

Rephrasing the ER: The highlighter color should be persistent.

Comment 2

[Patrick Brosset <:pbro>]

The reason it does this is because we're using the window selection API to highlight text ranges.
So the font highlighter basically creates actual text selections. Just like if you would select text yourself in a web page.
This means that the selections are gray when the content window doesn't have focus. And they're whatever is your custom selection color when it windows does have focus.

We could experiment with the CSS ::selection pseudo-element styling to see if we can make the color always be the same.

If that doesn't work, the other solution is to remove our usage of the selection API and draw some rectangle on the screens.
Some disadvantages though:
- there may be many text runs, so many things to draw, which may have an impact on performance
- we would have to calculate all of the text runs' coordinates ourselves and position everything correctly (page zoom, scrollable areas, iframes, etc. tend to make this complex, but doable)
- if the text is CSS transformed, we'd have to support this too.
Mind you, we do this for our other highlighters, so it's doable. But here we have a choice of using the selection API which makes our lives so much easier.

Comment 3

[Jonathan Kew [:jfkthame]]

I notice that when ::selection is used to provide custom selection color/background, both Safari and Chrome maintain the custom colors from CSS even when the element loses focus. Firefox currently switches to the default inactive-selection colors in that case, so the custom colors are lost.

According to https://drafts.csswg.org/css-pseudo-4/#selectordef-selection there's supposed to be an ::inactive-selection pseudo that would be applicable here, but none of webkit/blink/gecko appear to implement that at the moment. The gecko issue for that is bug 1475773.

In the meantime, there's a difference between webkit/blink and gecko regarding whether ::selection applies to inactive elements; they say yes, we say no. Maybe we could just change our behavior here to align with Safari/Chrome, which would resolve this issue for you?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Jonathan Kew (:jfkthame) from comment #3)

In the meantime, there's a difference between webkit/blink and gecko regarding whether ::selection applies to inactive elements; they say yes, we say no. Maybe we could just change our behavior here to align with Safari/Chrome, which would resolve this issue for you?

Oh cool, it indeed works great on webkit/blink. Aligning would definitely solve the issue for us!

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1479760 - Use the ::selection colors to paint inactive as well as active selections. r=emilio

This will enable the devtools font highlighting issue in bug 1479760 to be fixed
using ::selection, and matches webkit/blink behavior.

When we implement ::inactive-selection (bug 1475773) it will supersede this.

Comment 6

[Jonathan Kew [:jfkthame]]

:pbro, I believe the patch above will cause ::selection color/background to work consistently whether the element is active or inactive, so that you'd be able to use it for the devtools font highlighting. I'm not entirely sure whether there will be other less-desirable side effects (does anyone rely on our current behavior? I doubt it, actually, given that it's not interoperable with the other browsers), but if you can confirm this works for you we could consider taking it at least as an interim solution.

Comment 7

[Patrick Brosset <:pbro>]

While I can't test this patch right now because I don't have what it takes to do a full build, I can confirm that injecting a stylesheet that defines ::selection is something that works for devtools. So if this makes it take effect whether or not the element is active, I think this would be a perfect solution for us.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1479760 - Inject stylesheet with a custom ::selection background for the devtools font highlighter.

Comment 9

[Jonathan Kew [:jfkthame]]

Try build with the above patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e30a4cdbc0037f37a593d8f0a7ceaa043647dc4

Comment 10

[Patrick Brosset <:pbro>]

Thanks Jonathan for the patches. The DevTools changes look good, but can be made even simpler by re-using the (loadSheet)[https://searchfox.org/mozilla-central/source/devtools/shared/layout/utils.js#792] utility we already use in other places.

I'll do that part over in bug 1539989 as this is more in line with what Victoria requested there.
Let's keep this bug here for the platform changes only, related to making ::selection work on inactive elements.

Thanks!

Comment 11

[Jonathan Kew [:jfkthame]]

Sounds good, thanks. I'll just land the core Gecko patch here.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59e4bc8fa1ba
Use the ::selection colors to paint inactive as well as active selections. r=emilio

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out changeset for reftest failures on rtl-selection-with-decoration.html.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&tochange=dce6a59456288808e68e71ec339ee2b40a04dc37&fromchange=59e4bc8fa1ba799d9b7bc4db646174023b4e1ba6&searchStr=reftest&selectedJob=237360791

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=237360791&repo=autoland&lineNumber=1751
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=237361908&repo=autoland&lineNumber=2440

Backout link: https://hg.mozilla.org/integration/autoland/rev/dce6a59456288808e68e71ec339ee2b40a04dc37

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73388d92a46f
Use the ::selection colors to paint inactive as well as active selections. r=emilio

Comment 15

[Jonathan Kew [:jfkthame]]

FTR, marked the failing SVG selection tests as failing on Android for now, and filed bug 1541500 to address the underlying issue. (It's really a pre-existing bug, and got exposed because the change here affected the rendering of the reference files.)

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/73388d92a46f