Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

colorManagement in browserSettings #7893

Conversation

rebloor
Copy link
Contributor

@rebloor rebloor commented Aug 13, 2021

Documentation updates for the addition of support for colorManagement in browserSettings. Also aligned the title of contextMenuShowEvent to include browserSettings. as it is with the other browserSettings objects.

See Bug 1714428

Documentation updates for the addition of support for colorManagement in browserSettings. Also aligned the title of contextMenuShowEvent to include browserSettings.  as it is with the other browserSettings objects.

See   [Bug 1714428](https://bugzilla.mozilla.org/show_bug.cgi?id=1714428)
@rebloor rebloor requested a review from a team as a code owner August 13, 2021 16:49
@rebloor rebloor requested review from rpl and removed request for a team August 13, 2021 16:49
@sideshowbarker sideshowbarker added the Content:WebExt WebExtensions docs label Aug 13, 2021
@sideshowbarker
Copy link
Collaborator

The colormanagement directory name need to be all-lowercase:

files/en-us/mozilla/add-ons/webextensions/api/browsersettings/colormanagement/index.html

It’s currently colorManagement, and that’s what’s causing CI to fail at https://github.com/mdn/content/runs/3325118730#step:8:44

@rebloor
Copy link
Contributor Author

rebloor commented Aug 14, 2021

@sideshowbarker Making this slug all lowercase doesn't seem to have worked either. Other slugs appear to use casing e.g. Mozilla/Add-ons/WebExtensions/API/browserSettings/contextMenuShowEvent in the other file updated in this PR.

@sideshowbarker
Copy link
Collaborator

@sideshowbarker Making this slug all lowercase doesn't seem to have worked either. Other slugs appear to use casing e.g. Mozilla/Add-ons/WebExtensions/API/browserSettings/contextMenuShowEvent in the other file updated in this PR.

It’s not the slug that the CI is failing on — it’s the actual directory name on the filesystem.

The slugs should actually be mixed case, so that slug was already correct as it was.

@rebloor
Copy link
Contributor Author

rebloor commented Aug 14, 2021

@sideshowbarker Thanks, everything is green now on the tests, but I need approval again.

@sideshowbarker
Copy link
Collaborator

@sideshowbarker Thanks, everything is green now on the tests, but I need approval again.

I think it needs to be approved by @rpl or someone else from the @mdn/yari-content-mozilla-add-ons team

@Rumyra
Copy link
Collaborator

Rumyra commented Aug 16, 2021

Thanks @rebloor - are you adding the browser compat data for browsersettings.colormanagement ?

@rpl
Copy link
Member

rpl commented Aug 16, 2021

@rebloor lgtm, as @Rumyra mentioned in #7893 (comment) browserSettings.colorManagement has to be added to the browser compat data to ensure the browser compatibility table will be rendered as expected once this will be merged and deployed, and so it would be good to make sure that the compatibility data is available before releasing this change (I'm not sure if mdn does need that it is also in the @mdn/browser-compat-data package released on npm, @Rumyra may know more about that).

As a side note @mixedpuppy reviewed these changes to the browserSettings.colorManagement API and so he may know if there are some more details that would be worth adding.

@Rumyra
Copy link
Collaborator

Rumyra commented Aug 16, 2021

Thanks @rpl 👍

Apologies for the slightly blunt compat data comment 🙏

@rebloor this is the browser compat file that needs updating https://github.com/mdn/browser-compat-data/blob/main/webextensions/api/browserSettings.json with the addition of colorManagement. Once that's been added the compatibility section of this page will display the appropriate table :)

@rebloor
Copy link
Contributor Author

rebloor commented Aug 16, 2021

@Rumyra The compatibility table were there all along :-)

@Rumyra
Copy link
Collaborator

Rumyra commented Aug 16, 2021

Hah - thank you @rebloor :D I'm happy to merge, but I'll wait to see if @mixedpuppy has any comments 👍

@rpl
Copy link
Member

rpl commented Aug 16, 2021

Hah - thank you @rebloor :D I'm happy to merge, but I'll wait to see if @mixedpuppy has any comments +1

@Rumyra I just signed off it from our (Add-ons team) perspective (also briefly talked about it with @mixedpuppy to let him know this was pending sign-off on our side and that I was going to approve it in a few minutes).

@mixedpuppy mixedpuppy self-requested a review August 16, 2021 19:32
@mixedpuppy
Copy link
Collaborator

@mkaply can you add to the docs an explanation why this setting may be used?

Copy link
Collaborator

@mixedpuppy mixedpuppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asking for additional doc

@rebloor
Copy link
Contributor Author

rebloor commented Aug 19, 2021

@mkaply Do you have any information on why developers would use the settings?

@mkaply
Copy link

mkaply commented Aug 19, 2021

Yes, sorry.

This is useful when you are viewing images or videos in the browser and you want to view them using their original color profile as opposed to having the browser or operating system modify the color.

@rebloor
Copy link
Contributor Author

rebloor commented Aug 24, 2021

@mixedpuppy The additional information you asked for is now there. Please let me know if is anything else.

@rebloor rebloor requested a review from a team as a code owner August 27, 2021 16:36
@rebloor rebloor requested review from Rumyra, mixedpuppy and rpl and removed request for a team August 27, 2021 16:36

<p>A {{WebExtAPIRef("types.BrowserSetting", "BrowserSetting")}} object used to query and set the browser's color management features.</p>

<p>By default, Firefox applies color management to tagged media and defaults to sRGB for untagged media. This behavior means that some untagged media, such as that used in animation and movie production, may have undesired color corrections made to it. Use these settings to prevent that. However, note that these are browser settings, so you may want to modify them only while you're extension is active.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear what "However, note that these are browser settings, so you may want to modify them only while you're extension is active." means. I assume that the first part means "Changing this setting affects the entire browser". I'm not sure if "active" refers to the enabled state of the addon, or if it means that the addon is e.g. making the change for something in a specific tab. If the former, when we disable the addon, all its settings are undone, so the addon doesn't need to worry about that.

@mkaply I also don't know what "tagged media" is. Is there a way for an addon to tag media so that it can prevent that media from having sRGB applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mixedpuppy

I've removed the "browser wide" comment given that this should be adequately covered by the comments on API's introductory page.

For information on "tagged media." This is information embedded in the image or video to indicate the profile to apply when it's rendered. I couldn't find a really good reference, but this might help: https://www.graphicartsmedia.com/magazine-stand/stabilizing-colour-management-with-varying-web-browsers/. So by default, if the image isn't tagged Firefox applies sRGB, otherwise it applies the color profile tagged in the image. Does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mixedpuppy Please let me know if you're request for changes has been addressed, thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we link "tagged media" to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/3.5/ICC_color_correction_in_Firefox ? r+ with that (or better link if one is avialable)

@rebloor rebloor merged commit 6624610 into mdn:main Sep 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Content:WebExt WebExtensions docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants