Closed Bug 1479760 Opened Last year Closed 5 months ago

Font Highlighter changes color when interacting with page

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: mbalfanz, Assigned: jfkthame)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached video 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)
Rephrasing the ER: The highlighter color should be persistent.
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.
Severity: normal → enhancement
Priority: -- → P3
See Also: → 1539989

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?

Depends on: 1475773

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

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.

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

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.

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!

Assignee: nobody → jfkthame
Component: Inspector: Fonts → Layout: Text and Fonts
Product: DevTools → Core
Status: NEW → ASSIGNED
Attachment #9054614 - Attachment is obsolete: true

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

Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59e4bc8fa1ba
Use the ::selection colors to paint inactive as well as active selections. r=emilio
Attachment #9054483 - Attachment description: Bug 1479760 - Use the ::selection colors to paint inactive as well as active selections. → Bug 1479760 - Use the ::selection colors to paint inactive as well as active selections. r=emilio
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73388d92a46f
Use the ::selection colors to paint inactive as well as active selections. r=emilio

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

Flags: needinfo?(jfkthame)
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.