Bug 224389 – Add support for a `media` attribute on `<meta name="theme-color" content="...">`

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

[Devin Rousso]

see <https://github.com/whatwg/html/pull/6569>

[Devin Rousso]

<rdar://problem/74991621>

[Devin Rousso]

Created attachment 425761 [details]
Patch

[Sam Weinig]

Comment on attachment 425761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425761&action=review

It would be good to have a test for 'currentcolor', since the spec has special rules for that https://html.spec.whatwg.org/#parsed-as-a-css-color-value.

> Source/WebCore/html/HTMLMetaElement.cpp:110
> +            if (equalLettersIgnoringASCIICase(this->name(), "theme-color") && mediaAttributeMatches() && !contentColor().isValid() && CSSParser::parseColor(oldValue).isValid())

It seems unfortunate to reparse the old value. Anyway to avoid that?

[Devin Rousso]

Comment on attachment 425761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425761&action=review

(In reply to Sam Weinig from comment #3)
> It would be good to have a test for 'currentcolor', since the spec has special rules for that https://html.spec.whatwg.org/#parsed-as-a-css-color-value.
We currently don't support `currentcolor` for this (see <rdar://74814749>), but I can work on adding it.

>> Source/WebCore/html/HTMLMetaElement.cpp:110
>> +            if (equalLettersIgnoringASCIICase(this->name(), "theme-color") && mediaAttributeMatches() && !contentColor().isValid() && CSSParser::parseColor(oldValue).isValid())
> 
> It seems unfortunate to reparse the old value. Anyway to avoid that?

Yeah I can try restructuring this function a bit so that we check it before `HTMLElement::attributeChanged` (which calls `HTMLMetaElement::parseAttribute`, which clears `m_contentColor`).

[Theresa O'Connor]

(In reply to Sam Weinig from comment #3)

> It would be good to have a test for 'currentcolor', since the spec has
> special rules for that
> https://html.spec.whatwg.org/#parsed-as-a-css-color-value.

The spec PR for this doesn't depend on that algorithm: https://github.com/whatwg/html/pull/6569/files

[Theresa O'Connor]

(In reply to Theresa O'Connor from comment #5)
> (In reply to Sam Weinig from comment #3)
> 
> > It would be good to have a test for 'currentcolor', since the spec has
> > special rules for that
> > https://html.spec.whatwg.org/#parsed-as-a-css-color-value.
> 
> The spec PR for this doesn't depend on that algorithm:
> https://github.com/whatwg/html/pull/6569/files

Whoops, sorry, the existing spec text *does* depend on that. The PR doesn't change that.

[Sam Weinig]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 425761 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425761&action=review
> 
> (In reply to Sam Weinig from comment #3)
> > It would be good to have a test for 'currentcolor', since the spec has special rules for that https://html.spec.whatwg.org/#parsed-as-a-css-color-value.
> We currently don't support `currentcolor` for this (see <rdar://74814749>),
> but I can work on adding it.

Please don't link just to radar links in bugzilla, many of our contributors cannot see them. If there is work that is only in radar, it is probably worth making a bugzilla bug for it.

[Sam Weinig]

(In reply to Theresa O'Connor from comment #6)
> (In reply to Theresa O'Connor from comment #5)
> > (In reply to Sam Weinig from comment #3)
> > 
> > > It would be good to have a test for 'currentcolor', since the spec has
> > > special rules for that
> > > https://html.spec.whatwg.org/#parsed-as-a-css-color-value.
> > 
> > The spec PR for this doesn't depend on that algorithm:
> > https://github.com/whatwg/html/pull/6569/files
> 
> Whoops, sorry, the existing spec text *does* depend on that. The PR doesn't
> change that.

The tests don't have to pass, I just think there should be tests.

[Devin Rousso]

Created attachment 425913 [details]
Patch

(In reply to Sam Weinig from comment #8)
> (In reply to Theresa O'Connor from comment #6)
> > (In reply to Theresa O'Connor from comment #5)
> > > (In reply to Sam Weinig from comment #3)
> > > 
> > > > It would be good to have a test for 'currentcolor', since the spec has special rules for that https://html.spec.whatwg.org/#parsed-as-a-css-color-value.
> > > 
> > > The spec PR for this doesn't depend on that algorithm: https://github.com/whatwg/html/pull/6569/files
> > 
> > Whoops, sorry, the existing spec text *does* depend on that. The PR doesn't change that.
> 
> The tests don't have to pass, I just think there should be tests.

Personally I'd rather add the test when adding the feature.  I've created a followup <https://webkit.org/b/224509>.

[Devin Rousso]

Created attachment 426860 [details]
Patch

rebase with some minor adjustments

[Devin Rousso]

Created attachment 426861 [details]
Patch

oops forgot tests

[Darin Adler]

Comment on attachment 426861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426861&action=review

> Source/WebCore/dom/Document.h:746
> +    enum class RecalculateThemeColor : uint8_t { No, IfNeeded, Yes };
> +    const Color& themeColor(RecalculateThemeColor = RecalculateThemeColor::IfNeeded);

These are three separate functions. They should *not* be a single function with an enum argument.

Compute the theme color (should be private), return the theme color (computed and cached as needed, should be public), or return the old stored version (should be private, and maybe need not be a function at all, just access to a data member).

[Darin Adler]

Comment on attachment 426861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426861&action=review

> Source/WebCore/html/HTMLMetaElement.cpp:127
> +    bool wasValidMedia = m_media && mediaAttributeMatches();
> +    bool wasValidColor = m_contentColor && contentColor().isValid();
> +
>      HTMLElement::attributeChanged(name, oldValue, newValue, reason);
>  
> -    if (name == nameAttr && equalLettersIgnoringASCIICase(oldValue, "theme-color") && !equalLettersIgnoringASCIICase(newValue, "theme-color"))
> -        document().processMetaElementThemeColor(emptyString());
> +    // Changing a meta tag while it's not in the tree shouldn't have any effect on the document.
> +    if (!isConnected())
> +        return;
> +
> +    // An attribute being added (i.e. if the `media` attribute didn't exist) is handled by `process()`.
> +    if (!wasValidMedia || !wasValidColor)
> +        return;
> +
> +    if (name == nameAttr) {
> +        if (equalLettersIgnoringASCIICase(oldValue, "theme-color") && !equalLettersIgnoringASCIICase(newValue, "theme-color"))
> +            document().metaElementThemeColorChanged();
> +        return;
> +    }
> +
> +    if (name == contentAttr) {
> +        if (equalLettersIgnoringASCIICase(this->name(), "theme-color") && !contentColor().isValid())
> +            document().metaElementThemeColorChanged();
> +        return;
> +    }
> +
> +    if (name == mediaAttr) {
> +        if (equalLettersIgnoringASCIICase(this->name(), "theme-color") && !mediaAttributeMatches())
> +            document().metaElementThemeColorChanged();
> +        return;
> +    }

Not really great that this code is finely tuned based on the algorithm for what affects the color. Probably better to instead call metaElementThemeColorChanged and have it notice nothing changed. Too much logic here that replicates rules about the color that are also elsewhere. The important optimization is not telling the client outside WebKit that the color changed when it didn’t. Within WebKIt it’s better not to have logic spread across in multiple places. Just a list of which attributes might affect it should be enough, not the value.

I think the best design might be just to call:

    document().metaElementAttributeChanged();

And call it unconditionally. Leave out all the rest of the logic here. Or maybe put all the notification inside things called by process().

[Devin Rousso]

Comment on attachment 426861 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426861&action=review

>> Source/WebCore/dom/Document.h:746
>> +    const Color& themeColor(RecalculateThemeColor = RecalculateThemeColor::IfNeeded);
> 
> These are three separate functions. They should *not* be a single function with an enum argument.
> 
> Compute the theme color (should be private), return the theme color (computed and cached as needed, should be public), or return the old stored version (should be private, and maybe need not be a function at all, just access to a data member).

Yeah, I'm not really sure what I was thinking when I did this 😅

>> Source/WebCore/html/HTMLMetaElement.cpp:127
>> +    }
> 
> Not really great that this code is finely tuned based on the algorithm for what affects the color. Probably better to instead call metaElementThemeColorChanged and have it notice nothing changed. Too much logic here that replicates rules about the color that are also elsewhere. The important optimization is not telling the client outside WebKit that the color changed when it didn’t. Within WebKIt it’s better not to have logic spread across in multiple places. Just a list of which attributes might affect it should be enough, not the value.
> 
> I think the best design might be just to call:
> 
>     document().metaElementAttributeChanged();
> 
> And call it unconditionally. Leave out all the rest of the logic here. Or maybe put all the notification inside things called by process().

Is there also a performance concern of crawling the entire DOM to find all `HTMLMetaElement`?  That's mainly what I'm trying to avoid here by not notifying the `Document` if the change wouldn't affect `m_metaThemeColorElements` held by the `Document` (e.g. if the color was invalid previously and still is invalid).

Perhaps this is a premature "optimization" though and we could just do as you suggest and always notify the `Document` whenever `content`/`media` changes for any `<meta name="theme-color">` and have `Document` deal with filtering out cases that don't need to notify the UIProcess.  I'll give it a shot.

[Devin Rousso]

Created attachment 427323 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 427323 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427323&action=review

> Source/WebCore/html/HTMLMetaElement.cpp:93
> +    // Changing a meta tag while it's not in the tree shouldn't have any effect on the document.

This comment is unnecessary. It's pretty obvious from the code that's what's happening.

[Devin Rousso]

Created attachment 428044 [details]
Patch

[EWS]

Committed r277270 (237537@main): <https://commits.webkit.org/237537@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428044 [details].