1427107 - Invalid badge background color behaves as null

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[Oriol Brufau [:Oriol]]

Attachment: test.xpi

Run this code:

  browser.browserAction.setBadgeBackgroundColor({color: "red"});
  browser.browserAction.setBadgeBackgroundColor({tabId, color: "green"});
  browser.browserAction.setBadgeBackgroundColor({tabId, color: "invalid"});

Since "invalid" is not a color, it behaves as null, i.e. it removes the tab-specific green color and the tab inherits the global red color.

However, Chrome behaves differently. There the "invalid" color is ignored, so the tab continues having a green tab-specific color. And a warning is logged.

I think compatibility with Chrome should be preferred, if someone wants to remove a tab-specific color, since bug 1419940 they can explicitly use null, which is less hacky than an invalid color.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: test.xpi

There was a mistake in the attached test webext

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: Bug 1427107 - Reject setBadgeBackgroundColor promise for invalid string colors

Review commit: https://reviewboard.mozilla.org/r/212120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212120/

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8941904 [details]
Bug 1427107 - Reject setBadgeBackgroundColor promise for invalid string colors

https://reviewboard.mozilla.org/r/212120/#review218914

Comment 4

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ee783915e508a3257642d4fc4eb183b020963b09 -d 7be74a94e766: rebasing 442801:ee783915e508 "Bug 1427107 - Reject setBadgeBackgroundColor promise for invalid string colors r=mixedpuppy" (tip)
merging browser/components/extensions/ext-browserAction.js
merging browser/components/extensions/test/browser/browser_ext_browserAction_context.js
warning: conflicts while merging browser/components/extensions/ext-browserAction.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 5

[Oriol Brufau [:Oriol]]

Comment on attachment 8941904 [details]
Bug 1427107 - Reject setBadgeBackgroundColor promise for invalid string colors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212120/diff/1-2/

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The code base has been updated and the patch fails to apply now. Please push fix the issue and push the updated patch to review board. Thank you.

Comment 7

[Oriol Brufau [:Oriol]]

Done!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc54bd9b08c8
Reject setBadgeBackgroundColor promise for invalid string colors r=mixedpuppy

Comment 9

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/bc54bd9b08c8

Comment 10

[marius.santa]

Attachment: background color null.gif

Tested with the latest build and the only difference I see from before the fix is that the color of the "abcd" background is red by default and in the latest fix it's green. Is this the desired behavior along with the "Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page." warning?

Comment 11

[Oriol Brufau [:Oriol]]

Yes, the desired behavior is that the badge background color should be green on the tab where you installed the extension, just like Chrome. The event pages warning is unrelated.

Comment 12

[marius.santa]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1427107#c10 setting this issue to verified on Windows 10 64 Bit Firefox 60.0a1 (20180123102017).