Page MenuHomePhabricator

Bug 1803355: Basic implementation of Custom Highlight API. r=emilio,edgar
ClosedPublic

Authored by jjaschke on Dec 8 2022, 12:06 PM.
Referenced Files
Unknown Object (File)
Jan 22 2026, 3:50 PM
Unknown Object (File)
Nov 13 2025, 9:08 AM
Unknown Object (File)
Nov 13 2025, 9:08 AM
Unknown Object (File)
Nov 13 2025, 9:07 AM
Unknown Object (File)
Nov 13 2025, 9:07 AM
Unknown Object (File)
Nov 13 2025, 9:07 AM
Unknown Object (File)
Nov 13 2025, 9:07 AM
Unknown Object (File)
Nov 13 2025, 9:07 AM

Details

Summary

Added WebIDL interfaces as per spec, added some necessary changes to support maplike and setlike structures to be accessed from C++.

Added ::highlight(foo) pseudo element to CSS engine.

Implemented Highlight as new kind of Selection using HighlightType::eHighlight. This implies Selections being added/removed during runtime (one Selection object per highlight identifier), therefore a dynamic container for highlight Selection objects was added to nsFrameSelection. Also, the painting code queries the highlight style for highlight Selections.

Implementation is currently hidden behind a pref dom.customHighlightAPI.enabled.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
devtools/server/actors/page-style.js
738 ↗(On Diff #667496)

You can see the inspector styles for UA rules as above by going to "Settings -> Show browser styles".

So, I guess the extra "rule" is the ::highlight pseudo-element, and we're somehow coming up with matches when you pass it down to InspectorUtils.getCSSStyleRules(element, "::highlight")?

There are two things going wrong:

 * We're parsing ::highlight(foo) even without the pref enabled. That needs to be fixed, see other comments.

  • We're not failing to parse ::highlight as a pseudo-element. We need to reject this, this is web-observable via getComputedStyle(element, "::highlight"), which shouldn't return a style.
  • We're somehow selector-matching the ::highlight styles? How?
744 ↗(On Diff #667496)

Can you file a bug to make this also apply to modal dialogs? We should show the backdrop styles there.

dom/base/AbstractRange.h
133

Let's make this const, since it doesn't change during the lifetime of the object.

dom/base/Highlight.h
110

This eventually would need to invalidate or so, right?

122

This eventually would need to invalidate or so, right?

layout/generic/nsIFrame.cpp
2415

Why would we want to disregard these in high-contrast-mode? For selection it makes sense because we want the "native" selection style. But what's the default style for highlights?

layout/generic/nsTextFrame.cpp
6353

You need to null out aHighlightName otherwise right?

modules/libpref/init/StaticPrefList.yaml
2264

This only gates the DOM API but not the CSS one. So this needs to become a type: RelaxedAtomicBool (because we parse CSS off the main thread), and needs to use rust: true.

servo/components/style/gecko/pseudo_element_definition.mako.rs
30

Maybe add an is_simple() getter that does the not (is_tree or is_highlight)? You're using that in a couple places.

servo/components/style/gecko/selector_parser.rs
447

Definitely needs to check is_pseudo_element_enabled, and you need to add a pref check in here.

Something like:

if self.is_highlight() && !pref!("...") {
    return false;
}
This revision now requires changes to proceed.Jan 12 2023, 2:09 PM
jjaschke updated this revision to Diff 667943.
jjaschke marked 10 inline comments as done.

Code analysis found 1 defect in the diff 667943:

  • 1 defect found by py-black (Mozlint)
IMPORTANT: Found 1 issue (error level) that must be fixed before landing.

You can run this analysis locally with:

  • ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 667943.

devtools/server/actors/page-style.js
738 ↗(On Diff #667496)

Seems to be fixed after correctly querying the pref.

744 ↗(On Diff #667496)

I can't follow...

devtools/server/tests/chrome/test_styles-applied.html
92 ↗(On Diff #667496)

Fixed by correctly querying the pref in the CSS code. Changes are reverted.

dom/base/Highlight.h
110

Priorities and types are not supported yet in this patch. But yes, when one of these properties are changed during runtime, this needs to be propagated (which in worst case could mean remove the selection and add it again).

layout/generic/nsIFrame.cpp
2415

The default style of a highlight is "do nothing" (not sure if that answers the question or if my code is correct in that case)

1 issue closed compared to the previous diff 667943.


If you see a problem in this automated review, please report it here.

dom/base/Document.cpp
6898–6900

Doesn't

if (mHighlightRegistry) {
  mHighlightRegistry->AddHighlightSelectionsToFrameSelection(aRv);
}

work?

dom/base/HighlightRegistry.cpp
71

It looks like there is a room to optimize how we handle the changes of highlight range, right now we always create a new selection and throw the old one away.
But it seems to me that we could just update the existing selection's range if any. I am okay to live with this for now, but please file a follow up bug for it. :)

161

nit: bail out earlier for readability.

if (foundIter == mHighlightsOrdered.cend()) {
  return;
}

RefPtr<Highlight> highlight = foundIter->second();
dom/base/HighlightRegistry.h
59–60

Does annotating Document::CreatePresShell() with MOZ_CAN_RUN_SCRIPT work? Then we don't need to expose this method.

jjaschke marked 5 inline comments as done.
edgar added a project: testing-approved.
edgar added 1 blocking reviewer(s): emilio.

Looks good to me, thanks!

smaug added inline comments.
dom/base/HighlightRegistry.cpp
32

this is missing to unlink the wrapper.
Add NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER

emilio requested changes to this revision.Jan 21 2023, 3:56 PM

Thanks, this looks good with comments addressed. Sorry for the lag reviewing this. Please address @smaug's comment too :)

dom/base/HighlightRegistry.cpp
64

Let's do:

RefPtr<nsFrameSelection> frameSelection = GetFrameSelection();
if (!frameSelection) {
  return;
}

// Deindent the whole thing?
88

Same here, let's deindent the loop?

145

Hmm, do we really want to early-return here?

But I took a look and this can't really fail, so let's just remove the ErrorResult arg...

layout/generic/nsFrameSelection.cpp
1592

This can't fail, let's remove aRv?

layout/generic/nsFrameSelection.h
49

This comment doesn't make sense tho, because nsAtom objects can't have cycles. So I'd just make this a RefPtr<const nsAtom> unless there's any performance concern.

layout/generic/nsIFrame.cpp
2415

Yeah I don't think we want to just render nothing if forcing colors. Let's remove this for now.

layout/style/ServoStyleSet.cpp
506

This is not true, so style should always be up-to-date here. Let's replace the UpdateStylistIfNeeded call by:

MOZ_ASSERT(!StylistNeedsUpdate());
servo/components/style/gecko/pseudo_element.rs
166

nit: extra newline?

servo/ports/geckolib/glue.rs
4061

this can just be:

let matching_fn = |pseudo: &PseudoElement| pseudo == pseudo_element;

right?

4076

Let's add an /* is_probe = */ true comment here

This revision now requires changes to proceed.Jan 21 2023, 3:56 PM
jjaschke updated this revision to Diff 671574.
jjaschke marked 11 inline comments as done.
jjaschke added inline comments.
dom/base/HighlightRegistry.cpp
71

Yes, this is intentional. I've not yet completely understood how this would work and this PR is more of a "let's get the stuff I already have finished reviewed and landed so we know it works/doesn't break anything, then let's deal with all the more complicated issues".
This goes along with supporting StaticRanges as well and will be a follow-up bug.

servo/ports/geckolib/glue.rs
4061

let matching_fn = |pseudo: &PseudoElement| pseudo == &pseudo_element;?

servo/ports/geckolib/glue.rs
4061

Ah, sure, or *pseudo == pseudo_element for that matter.

Alright, r=me with the nits below addressed. Thanks and sorry for the lag getting to this review :)

dom/base/Highlight.cpp
120
if (aRv.Failed()) {
  return;
}

Then deindent, to match also the check inside.

dom/base/HighlightRegistry.cpp
119

This check can be removed now as well.

153

Probably clearer with:

if (!Delete(..)) {
  return;
}
// Deindent the rest.
168

This check doesn't make sense now.

dom/base/nsISelectionController.idl
45

nit: space missing?

This revision is now accepted and ready to land.Jan 21 2023, 6:04 PM
jjaschke marked 7 inline comments as done.
This revision is now accepted and ready to land.Jan 22 2023, 3:06 AM
This revision is now accepted and ready to land.Jan 24 2023, 10:22 AM
This revision is now accepted and ready to land.Jan 24 2023, 4:57 PM