Closed
Bug 1437178
Opened 6 years ago
Closed 6 years ago
pageAction visibility issues when using show_matches and hide_matches
Categories
(WebExtensions :: General, defect, P2)
Tracking
(firefox-esr52 unaffected, firefox58 unaffected, firefox59 verified, firefox60 verified)
VERIFIED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | verified |
firefox60 | --- | verified |
People
(Reporter: Oriol, Assigned: Oriol)
References
Details
Attachments
(3 files, 2 obsolete files)
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
314 bytes,
application/x-xpinstall
|
Details | |
322 bytes,
application/x-xpinstall
|
Details |
Load attached webextension The manifest has "page_action": { "show_matches": ["<all_urls>"], "hide_matches": ["<all_urls>"] } hide_matches takes precedence over show_matches, so the pageAction should be hidden by default, but it's shown. https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/browser/components/extensions/ext-pageAction.js#44
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review224966 Well, that doesn't really make sense to do anyway. I'm curious if/how page action is usable in this situation. I'm guessing show still works?
Attachment #8949867 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #2) > Well, that doesn't really make sense to do anyway. I'm curious if/how page > action is usable in this situation. I'm guessing show still works? show_matches and hide_matches only determine the default shownness. show and hide are tab-specific, so they have more precedence.
Assignee | ||
Comment 4•6 years ago
|
||
Argh, in fact the problem is much worse, e.g. with "page_action": { "show_matches": ["<all_urls>"], "hide_matches": ["*://example.com/*"] } if I load example.com, the pageAction is shown. It's fixed when you switch to another tab and then back to the example.com tab. That's why the test didn't fail. In fact the test that I added seems useless because it also passes without the changes. Will think something else.
Keywords: checkin-needed
Assignee | ||
Comment 5•6 years ago
|
||
So I was thinking something like let show, showMatches, hideMatches; if (!show_matches.length || hide_matches.includes("<all_urls>")) { // Always hide by default show = false; } else if (show_matches.includes("<all_urls>") && !hide_matches.length) { // Always show by default show = true; } else { // Might show or hide depending on the URL showMatches = new MatchPatternSet(show_matches); hideMatches = new MatchPatternSet(hide_matches); } And then in updateButton if (show == null) { // Set show via pattern matching. let uri = tab.linkedBrowser.currentURI; show = tabData.showMatches.matches(uri) && !tabData.hideMatches.matches(uri); } this.browserPageAction.setDisabled(!show, window); But, <all_urls> is not supposed to match things like about:debugging, right? The above has the problem that an add-on with "page_action": { "show_matches": ["<all_urls>"] }, shows the pageAction everywhere, including about:debugging, but an add-on with "page_action": { "show_matches": ["<all_urls>"], "hide_matches": ["*://example.com/*"] }, does not show it in about:debugging. Currently it's shown in both cases. For consistency, even if show_matches contains <all_urls> and hide_matches is empty, pattern matching can't be avoided. But the case !show_matches.length || hide_matches.includes("<all_urls>") can still be optimized, which is good because I expect !show_matches.length to happen frequently. Currently it is not optimized! https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/browser/components/extensions/ext-pageAction.js#241 In fact it could be generalized to "every pattern in show_matches is superseded by or equal to some pattern in hide_matches", but not sure if it's worth checking because it would be pointless for an extension author to do that.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Oh, then isShown returns undefined for tabs where we haven't evaluated yet whether the pageAction should be shown or not. Could be a feature, but looks like a bug.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219184/#review225048 ::: browser/components/extensions/ext-browser.js (Diff revision 3) > - if (!(~stateFlags & (flags.STATE_IS_WINDOW | flags.STATE_START) || > - this.lastLocation.has(browser))) { > - this.lastLocation.set(browser, request.URI); > - } > - } > - I remove this code because it updates the last location of the browser, then onLocationChange does not see the change and does not emit the "location-change" event, so the page action is not updated.
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches I think with this it's completely fixed. I ran the updated test without the other changes, and there were 74 failures out of 248 checks. The current firefox behavior is so buggy: - If show_matches contains <all_urls>, the page action is always shown by default - This includes some about: pages, which are not matched by <all_urls> - This includes pages that match some hide_matches (but this is fixed when switching tabs) - Page actions are not shown in the active tabs when the extension is installed, even if they match some show_matches (if show_matches does not contain <all_urls>) - isShown does not return the right value in various cases - Some location-change events are not detected, so the page action may not be updated when navigating
Attachment #8949867 -
Flags: review+ → review?(mixedpuppy)
Assignee | ||
Updated•6 years ago
|
Summary: Webext with both show_matches and hide_matches set to <all_urls> should be hidden by default → pageAction visibiltiy issues when using show_matches and hide_matches
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review225234 ::: browser/components/extensions/ext-browser.js (Diff revision 3) > this.emit("tab-select", nativeTab); > this.emit("location-change", nativeTab); > } > } > > - onStateChange(browser, webProgress, request, stateFlags, statusCode) { This also affects browser and sidebar actions. I need to think about that more, but the other issues below need to be addressed. ::: browser/components/extensions/ext-pageAction.js:47 (Diff revision 3) > > - // If <all_urls> is present, the default is to show the page action. > - let show = !!options.show_matches && options.show_matches.includes("<all_urls>"); > - let showMatches = new MatchPatternSet(options.show_matches || []); > - let hideMatches = new MatchPatternSet(options.hide_matches || []); > + let show_matches = options.show_matches || []; > + let hide_matches = options.hide_matches || []; > + > + let show, showMatches, hideMatches; > + if (!show_matches.length || hide_matches.includes("<all_urls>")) { Show/hideMatches is now undefined, yet new code you added doesn't check for that. I'm inclined to think that they should always be set as before. showing pageActions should always default to false unless show_matches overrides that with all_urls. Now the "show" value can only be either undefined or false, thus this doesn't really make sense. I'm inclined towards just removing "show" and always using the match patterns. ::: browser/components/extensions/ext-pageAction.js:107 (Diff revision 3) > }, > })); > + > + // If the page action is only enabled in some URLs, check if it should > + // be shown in active tabs. > + if (show == null) { use "===". using == here was confusing for a moment, because there is no way show could be null. This code should also be unecessary if show === false. ::: browser/components/extensions/ext-pageAction.js:165 (Diff revision 3) > - let tabData = this.tabContext.get(window.gBrowser.selectedTab); > + let tab = window.gBrowser.selectedTab; > + let tabData = this.tabContext.get(tab); > let title = tabData.title || this.extension.name; > this.browserPageAction.setTitle(title, window); > this.browserPageAction.setTooltip(title, window); > - this.browserPageAction.setDisabled(!tabData.show, window); > + let show = this.isShown(tab, tabData); This now does the matching for every update, I'd like to make sure this isn't called unecessarily. That was the point of doing this in handleLocationChange.
Attachment #8949867 -
Flags: review?(mixedpuppy) → review-
Comment 12•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review225234 > use "===". using == here was confusing for a moment, because there is no way show could be null. This code should also be unecessary if show === false. i.e. use if (show !== false) instead to make this clear what it is doing.
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #11) > Show/hideMatches is now undefined, yet new code you added doesn't check for > that. Yes, I do. I use show==null > I'm inclined to think that they should always be set as before. > showing pageActions should always default to false unless show_matches > overrides that with all_urls. Now the "show" value can only be either > undefined or false, thus this doesn't really make sense. > I'm inclined towards just removing "show" and always using the match > patterns. That's unnecessary. The common case is no show_matches and no hide_matches. Then match patterns will never match, so that can be optimized out by setting the default show to false. So there are three states for show: - false. This means the page action is not shown. This happens by default if there is no show_matches or if hide_matches has <all_urls>. It can also be set in a tab via pageAction.hide(tabId), e.g. to override show_matches. - true. This means the page action is shown. This will never happen by default because <all_urls> does not really match all urls, but can be set in a tab via pageAction.show(tabId). - undefined. This means the page action may be shown or not depending on the tab URL. This is the default value when there are some show_matches and hide_matches does not contain <all_urls>. Currently can't be set in a tab. We can (and should!) entirely skip pattern matching in the first two cases. Further optimization: if show is undefined, the result of pattern matching is cached as a tab-specific value so that pattern matching does not happen again in that tab. > use "===". using == here was confusing for a moment, because there is no > way show could be null. This code should also be unecessary if show === > false. But ===null does not detect undefined. Do you prefer ===undefined? Or should I use null instead of undefined? > This now does the matching for every update, I'd like to make sure this > isn't called unecessarily. That was the point of doing this in > handleLocationChange. No, just once, because the result is cached in my patch. Currently the result is not cached properly, and each time you switch tabs, pattern matching is performed: https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/browser/components/extensions/ext-pageAction.js#241
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
Do you prefer it like this? I moved the pattern matching part outside of updateButton and I explicitly set show=null instead of undefined.
Comment 16•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #13) > (In reply to Shane Caraveo (:mixedpuppy) from comment #11) > > use "===". using == here was confusing for a moment, because there is no > > way show could be null. This code should also be unecessary if show === > > false. > > But ===null does not detect undefined. Do you prefer ===undefined? Or should > I use null instead of undefined? My earlier confusion as I said was entirely around this. Readability of the code is better if explicit. However, I think there are things that can be changed, I'll note those in rb.
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #11) > Comment on attachment 8949867 [details] > Bug 1437178 - Fix various pageAction visibiltiy issues when using > show_matches and hide_matches > > https://reviewboard.mozilla.org/r/219186/#review225234 > > ::: browser/components/extensions/ext-browser.js > (Diff revision 3) > > this.emit("tab-select", nativeTab); > > this.emit("location-change", nativeTab); > > } > > } > > > > - onStateChange(browser, webProgress, request, stateFlags, statusCode) { > > This also affects browser and sidebar actions. I need to think about that > more, but the other issues below need to be addressed. Only browserAction and pageAction use "location-change" event: https://searchfox.org/mozilla-central/search?q=%22location-change%22&case=false®exp=false&path= And effectively browserAction is also affected: 1. Extract the attached test2.xpi 2. Load it in about:debugging 3. Open a new tab 4. Click the browserAction, this will set a tab-specific badge text 5. Click some link in the new tab page. This is a navigation, but the badge is not cleared! 6. You can then navigate again, and this time it will be cleared as expected. So removing this code is good.
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review225330 ::: browser/components/extensions/ext-browser.js (Diff revision 4) > this.emit("tab-select", nativeTab); > this.emit("location-change", nativeTab); > } > } > > - onStateChange(browser, webProgress, request, stateFlags, statusCode) { I still want to know why onStateChange was necessary in the first place so we can be certain of its removal. ::: browser/components/extensions/ext-pageAction.js:57 (Diff revision 4) > + // This is the default value when there are some show_matches and hide_matches > + // doesn't contain <all_urls>. Can't be set in a tab. > + let show, showMatches, hideMatches; > + let show_matches = options.show_matches || []; > + let hide_matches = options.hide_matches || []; > + if (!show_matches.length || hide_matches.includes("<all_urls>")) { If there is no "show_matches" then "hide_matches" is basically a no-op. Since hide_matches overrides, setting that to all_urls also creates (what should be) a no-op, but in a confusing manor. The current situation is: show_matches: ["http://example.com"], hide_matches: ["<all_urls>"] will never result in the action being shown on example.com, but clearly the intention is that the action is shown on example.com. Yes, the author should not have added hide_matches, but the point is that we should always honor show_matches in this case. IMO Just remove all_urls from hide_matches (logging a warning is fine). As well, if there are no show_matches, then hide_matches shouldn't have any effect. show_matches: ["http://example.com/*"], hide_matches: ["<all_urls>"] should be the same has having: show_matches: ["http://example.com/*"], hide_matches: [] (or no hide_matches in manifest) Having: show_matches: [] (or no show_matches in manifest) hide_matches: [...anything] is a no-op, and we should probably warn or error out. let show, showMatches, hideMatches; if (options.show_matches.length) { let hide_matches = options.hide_matches || []; if (hide_matches.includes("<all_urls>") { hide_matches.delete("<all_urls>") } showMatches = new MatchPatternSet(options.show_matches); hideMatches = new MatchPatternSet(hide_matches); } ::: browser/components/extensions/ext-pageAction.js:117 (Diff revision 4) > }, > })); > + > + // If the page action is only enabled in some URLs, do pattern matching in > + // the active tabs and update the button if necessary. > + if (show == null) { Since the default is to hide, we only need to do this if there are showMatches. So "if (showMatches)". ::: browser/components/extensions/ext-pageAction.js:195 (Diff revision 4) > + // Checks whether the tab action is shown when the specified tab becomes active. > + // Does pattern matching if necessary, and caches the result as a tab-specific value. > + isShown(tab) { > + let tabData = this.tabContext.get(tab); > + let {show} = tabData; > + if (show != null) { !== undefined is fine here. However I think this function could look like: if (tabData.show === undefined) { tabData.show = tabData.showMatches && tabData.showMatches.matches(uri) && !tabData.hideMatches.matches(uri); } return tabData.show ::: browser/components/extensions/test/browser/browser_ext_pageAction_show_matches.js:50 (Diff revision 4) > + { > + "name": "Test hide_matches all_urls overrides all_urls.", > + "page_action": { > + "show_matches": ["<all_urls>"], > + "hide_matches": ["<all_urls>"], > + }, > + "shown": [false, false, false, false], > }, settings like this is nonsensical. Given my other suggested changes, this test should have show_matches with all_urls same as the first test. I don't think the extra test is necessary. ::: browser/components/extensions/test/browser/browser_ext_pageAction_show_matches.js:93 (Diff revision 4) > window.getComputedStyle(button).display == "none"; > is(!hidden, test.shown[t], `test ${i} tab ${t}: page action is ${test.shown[t] ? "shown" : "hidden"} for ${tab.linkedBrowser.currentURI.spec}`); > - } > + await sendMessage(extension, "isShown", {result: test.shown[t]}); > + }; > > + info("Check show_matches and hide_matches are respected when installing the extension"); I think this test has become confusing, it's trying to do too much. It seems we have two things we want to verify. The shown state of tabs on startup (active tab and otherwise), and the state upon tab switching. I'd split this in two test functions (perhaps just leave the original as-is but make the test data global).
Attachment #8949867 -
Flags: review?(mixedpuppy) → review-
Comment 19•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #17) > > > > > > - onStateChange(browser, webProgress, request, stateFlags, statusCode) { > So removing this code is good. I understand why the str is broken and why the change fixes *that* str. My point is that there is likely some other edge case that wasn't working until onStateChange was introduced.
Comment 20•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #19) > (In reply to Oriol Brufau [:Oriol] from comment #17) > > > > > > > > > - onStateChange(browser, webProgress, request, stateFlags, statusCode) { > > > So removing this code is good. > > I understand why the str is broken and why the change fixes *that* str. My > point is that there is likely some other edge case that wasn't working until > onStateChange was introduced. I am also open to the possibility there is not any reason for it, or that the reason no longer exists, but it needs a deeper look.
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #18) > Since hide_matches overrides, setting that to all_urls also creates (what > should be) a no-op, but in a confusing manor. Well, that's how this API was designed. I'm just fixing bugs. I'm surprised you don't like it since it seems it was you who designed it in bug 1419842 comment 18. > The current situation is: > > show_matches: ["http://example.com"], > hide_matches: ["<all_urls>"] > > will never result in the action being shown on example.com, but clearly the > intention is that the action is shown on example.com. Yes, the author > should not have added hide_matches, but the point is that we should always > honor show_matches in this case. > IMO Just remove all_urls from hide_matches (logging a warning is fine). I don't think we should assume webext authors are stupid and they meant different things than what they wrote. If an author does not know how hide_matches works, then it's better that we don't try to fix things so that they will notice the problem earlier. So I'm against ignoring <all_urls>, even with a warning. Blacklisting it so that the webext does not load if used in hide_matches would be more acceptable, but I don't see the need to do that. If you think some authors would prefer show_matches to have more precedence than hide_matches in some cases, then I think you should propose some kind of manifest flag to do that. That is, since show_matches: ["http://example.com/"], hide_matches: ["*://example.com/*"] is no-op, then I think show_matches: ["http://example.com/"], hide_matches: ["<all_urls>"] this should also be no-op instead of magically showing the page action in http://example.com/ Otherwise it's not consistent. > As well, if there are no show_matches, then hide_matches shouldn't have any > effect. Yes, this is what happens already. > ::: browser/components/extensions/ext-pageAction.js:117 > (Diff revision 4) > > }, > > })); > > + > > + // If the page action is only enabled in some URLs, do pattern matching in > > + // the active tabs and update the button if necessary. > > + if (show == null) { > > Since the default is to hide, we only need to do this if there are > showMatches. So "if (showMatches)". Uhm, in fact showMatches is thuthy if, and only if, the default show is null. So it doesn't really matter, but not sure if I follow your reasoning. For consistency I would prefer show===null since this is what must be checked in tab-specific show. > ::: browser/components/extensions/ext-pageAction.js:195 > (Diff revision 4) > > + // Checks whether the tab action is shown when the specified tab becomes active. > > + // Does pattern matching if necessary, and caches the result as a tab-specific value. > > + isShown(tab) { > > + let tabData = this.tabContext.get(tab); > > + let {show} = tabData; > > + if (show != null) { > > !== undefined is fine here. In the last update I switched to null. Do you want !==null or should I revert to undefined and use !==undefined? > However I think this function could look like: > > if (tabData.show === undefined) { > tabData.show = tabData.showMatches && > tabData.showMatches.matches(uri) && > !tabData.hideMatches.matches(uri); > } > return tabData.show If tabData.show is not a boolean then tabData.showMatches must be truthy, no need to check that. I was trying to avoid reading tabData.show multiple times, since it can be defined up in the prototype chain, but I guess it doesn't really matter. > ::: > browser/components/extensions/test/browser/ > browser_ext_pageAction_show_matches.js:50 > (Diff revision 4) > > + { > > + "name": "Test hide_matches all_urls overrides all_urls.", > > + "page_action": { > > + "show_matches": ["<all_urls>"], > > + "hide_matches": ["<all_urls>"], > > + }, > > + "shown": [false, false, false, false], > > }, > > settings like this is nonsensical. Given my other suggested changes, this > test should have show_matches with all_urls same as the first test. I don't > think the extra test is necessary. On the contrary, I think it's really necessary. Especially if you want <all_urls> in hide_matches to be handled in some special way.
Comment 22•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #21) > (In reply to Shane Caraveo (:mixedpuppy) from comment #18) > > Since hide_matches overrides, setting that to all_urls also creates (what > > should be) a no-op, but in a confusing manor. > > Well, that's how this API was designed. I'm just fixing bugs. I'm surprised > you don't like it since it seems it was you who designed it in bug 1419842 > comment 18. As the implementer I'm trying to explain the intention behind this so we can address it in a way that removes the potential confusion. I'd like to see all_urls removed from hide_matches one way or another. The schema can be modified to disallow all_urls which is probably the better way to go. Here's the change to make that work. Not fond of "MatchPatternRestricted" but not sure what a better name is right now. diff --git a/browser/components/extensions/schemas/page_action.json b/browser/components/extensions/schemas/page_action.json --- a/browser/components/extensions/schemas/page_action.json +++ b/browser/components/extensions/schemas/page_action.json @@ -37,17 +37,17 @@ "optional": true, "minItems": 1, "items": { "$ref": "MatchPattern" } }, "hide_matches": { "type": "array", "optional": true, "minItems": 1, - "items": { "$ref": "MatchPattern" } + "items": { "$ref": "MatchPatternRestricted" } } }, "optional": true } } } ] }, diff --git a/toolkit/components/extensions/schemas/manifest.json b/toolkit/components/extensions/schemas/manifest.json --- a/toolkit/components/extensions/schemas/manifest.json +++ b/toolkit/components/extensions/schemas/manifest.json @@ -431,16 +431,29 @@ }, { "type": "string", "pattern": "^file:///.*$" } ] }, { + "id": "MatchPatternRestricted", + "description": "Same as MatchPattern above, but excludes <all_urls>", + "choices": [ + { + "type": "string", + "pattern": "^(https?|wss?|file|ftp|\\*)://(\\*|\\*\\.[^*/]+|[^*/]+)/.*$" + }, + { + "type": "string", + "pattern": "^file:///.*$" + } + ] + }, + { "id": "MatchPatternInternal", "description": "Same as MatchPattern above, but includes moz-extension protocol", "choices": [ { "type": "string", "enum": ["<all_urls>"] }, {
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•6 years ago
|
||
I refactored the test, used strict equalities and changed the isShown function. Now I will do what you say in comment 22.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•6 years ago
|
||
I thought reusing MatchPatternRestricted in MatchPattern was better, but I will do your proposal if you prefer it
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review225752 r- only due to the onStateChange comment below. Other stuff is just a couple comment cleanups. ::: browser/components/extensions/ext-browser.js (Diff revision 6) > this.emit("tab-select", nativeTab); > this.emit("location-change", nativeTab); > } > } > > - onStateChange(browser, webProgress, request, stateFlags, statusCode) { Ok, so this was added in bug 1287649. Based on the patch in that bug, I don't see an obvious reason this would cause a problem with the show/hide patch here, but the particular reason for the "lastLocation" stuff is important, we don't want to regress that. ::: browser/components/extensions/ext-pageAction.js:50 (Diff revision 6) > - let showMatches = new MatchPatternSet(options.show_matches || []); > - let hideMatches = new MatchPatternSet(options.hide_matches || []); > + // It's set as default if show_matches is empty. Can also be set in a tab via > + // `pageAction.hide(tabId)`, e.g. in order to override show_matches. > + // - `true`. This means the page action is shown. > + // It's never set as default because <all_urls> doesn't really match all URLs > + // (e.g. "about:" URLs). But can be set in a tab via `pageAction.show(tabId)`. > + // - `null`. This means the visivility of the page action may depends on the tab URL. First sentence is unecessary, the following two sentences are quite specific and enough. ::: browser/components/extensions/ext-pageAction.js:190 (Diff revision 6) > } > this.browserPageAction.setIconURL(iconURL, window); > } > > + // Checks whether the tab action is shown when the specified tab becomes active. > + // Does pattern matching if necessary, and caches the result as a tab-specific value. Just add a comment that show can only be null if matches were configured.
Attachment #8949867 -
Flags: review?(mixedpuppy) → review-
Assignee | ||
Comment 28•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #27) > Ok, so this was added in bug 1287649. Based on the patch in that bug, I > don't see an obvious reason this would cause a problem with the show/hide > patch here, but the particular reason for the "lastLocation" stuff is > important, we don't want to regress that. OK, I will look into that. Or maybe this should be moved into another bug, but without removing that code my test failed. I'm not sure if the test from bug 1287649 works well. My patch didn't fail everywhere in treeherder, and where it failed it was marked as intermittent.
Comment 29•6 years ago
|
||
> ::: browser/components/extensions/ext-pageAction.js:50
> (Diff revision 6)
> > - let showMatches = new MatchPatternSet(options.show_matches || []);
> > - let hideMatches = new MatchPatternSet(options.hide_matches || []);
> > + // It's set as default if show_matches is empty. Can also be set in a tab via
> > + // `pageAction.hide(tabId)`, e.g. in order to override show_matches.
> > + // - `true`. This means the page action is shown.
> > + // It's never set as default because <all_urls> doesn't really match all URLs
> > + // (e.g. "about:" URLs). But can be set in a tab via `pageAction.show(tabId)`.
> > + // - `null`. This means the visivility of the page action may depends on the tab URL.
>
> First sentence is unecessary, the following two sentences are quite specific
> and enough.
Looks like some context is missing in my comment above. This is about the comments for "null". There are two more sentences after the last line rb sent.
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #29) > Looks like some context is missing in my comment above. This is about the > comments for "null". There are two more sentences after the last line rb > sent. So you want me to remove "This means the visivility of the page action may depends on the tab URL.", right? It has a typo anyways
Comment 31•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #30) > (In reply to Shane Caraveo (:mixedpuppy) from comment #29) > > Looks like some context is missing in my comment above. This is about the > > comments for "null". There are two more sentences after the last line rb > > sent. > > So you want me to remove "This means the visivility of the page action may > depends on the tab URL.", right? It has a typo anyways yes
Comment 32•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #28) > (In reply to Shane Caraveo (:mixedpuppy) from comment #27) > > Ok, so this was added in bug 1287649. Based on the patch in that bug, I > > don't see an obvious reason this would cause a problem with the show/hide > > patch here, but the particular reason for the "lastLocation" stuff is > > important, we don't want to regress that. > > OK, I will look into that. Or maybe this should be moved into another bug, > but without removing that code my test failed. > I'm not sure if the test from bug 1287649 works well. My patch didn't fail > everywhere in treeherder, and where it failed it was marked as intermittent. No, we don't remove necessary code to land something. It needs to be validated as unnecessary (with tests) or fixed (can be fixed in a separate bug, but as a blocker to landing this patch). I also did not like the looks of the test in that bug, but didn't dig further.
Assignee | ||
Comment 33•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #32) > No, we don't remove necessary code to land something. It needs to be > validated as unnecessary (with tests) or fixed (can be fixed in a separate > bug, but as a blocker to landing this patch). I also did not like the looks > of the test in that bug, but didn't dig further. Yes, I meant this bug could land the other fixes and then in another bug fix that the pageAction and browserAction are not always updated when they should. Or in reversed order.
Comment 34•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #33) > (In reply to Shane Caraveo (:mixedpuppy) from comment #32) > > No, we don't remove necessary code to land something. It needs to be > > validated as unnecessary (with tests) or fixed (can be fixed in a separate > > bug, but as a blocker to landing this patch). I also did not like the looks > > of the test in that bug, but didn't dig further. > > Yes, I meant this bug could land the other fixes and then in another bug fix > that the pageAction and browserAction are not always updated when they > should. Or in reversed order. Ah, ok. Problem is, if your tests don't pass (due to the other bug) it will end up getting backed out. So it's best to address the other issue and land it first.
Updated•6 years ago
|
Priority: -- → P2
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review226638 Given that bug 1438274 clears up the onStateChange issue, I think this is good after resolving any last nits.
Attachment #8949867 -
Flags: review- → review+
Assignee | ||
Comment 36•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #35) > Given that bug 1438274 clears up the onStateChange issue, I think this is > good after resolving any last nits. In fact it needs some further changes, because since bug 1438274 it's possible for the location to change without clearing the tab data. This means the result of pattern matching can't be stored as "show", because we need to distinguish the result of a previous pattern matching that might be now obsolete from the value store by show() or hide().
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•6 years ago
|
||
So now "show" contains the value set by show(), hide() or set by default because of an empty show_matches. And "patternMatching" caches the result of pattern matching, which has less precedence. Choosing between them was easier if using a destructuring assignment with a default, so the not-set value is now undefined instead of null. But I still used strict equalities. I added a test about history.pushState
Assignee | ||
Comment 39•6 years ago
|
||
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches Try seems green
Attachment #8949867 -
Flags: review+ → review?(mixedpuppy)
Comment 40•6 years ago
|
||
mozreview-review |
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches https://reviewboard.mozilla.org/r/219186/#review226914 LGTM! Thanks! ::: browser/components/extensions/ext-pageAction.js:177 (Diff revision 7) > this.browserPageAction.setTitle(title, window); > this.browserPageAction.setTooltip(title, window); > - this.browserPageAction.setDisabled(!tabData.show, window); > > + // At least one of "show" or "patternMatching" must be defined here. > + let {show = tabData.patternMatching} = tabData; This does require that one of those has been set prior to updateButton, otherwise it can result in show === undefined. However that results in the button being disabled, so I'm not really concerned.
Attachment #8949867 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 41•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #40) > This does require that one of those has been set prior to updateButton, > otherwise it can result in show === undefined. However that results in the > button being disabled, so I'm not really concerned. Yes, calling isShown ensures one will be defined. The only case where updateButton is called without checking isShown is in setProperty, but this only happens if the tab is active. And I covered both cases of becoming active and being already active when the extension is installed. But yes, if I missed some edge case, the worst that will happen is that the button will be hidden. Bug 1438274 landed, let's land this now.
Keywords: checkin-needed
Comment 42•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/a46fa982caed Fix various pageAction visibiltiy issues when using show_matches and hide_matches r=mixedpuppy
Keywords: checkin-needed
Comment 43•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a46fa982caed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Assignee | ||
Comment 44•6 years ago
|
||
Should this (and bug 1438274) be uplifted to fix the initial implementation of show_matches and hide_matches, or do you prefer to ride the trains?
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 46•6 years ago
|
||
Attachment #8949863 -
Attachment is obsolete: true
Attachment #8950324 -
Attachment is obsolete: true
Assignee | ||
Comment 47•6 years ago
|
||
Steps to reproduce: 1. Disable xpinstall.signatures.required in about:config 2. Load https://bugzilla.mozilla.org in a tab 3. In that tab, go to https://bugzilla.mozilla.org/attachment.cgi?id=8952160 4. Install the extension Expected: You should see the page action just after install (the location matches show_matches) 5. Open a new tab 6. Load https://bugzilla.mozilla.org Expected: You should see the page action (the location matches show_matches) 7. Remove the extension in about:addons and restore xpinstall.signatures.required 8. In about:debugging, load the attached test4.xpi Expected: You should not see the page action in about:debugging (the location does not match show_matches) 9. Open a new tab 10. Load http://example.com/ Expected: You should not see the page action in about:debugging (the location matches hide_matches)
Assignee | ||
Comment 48•6 years ago
|
||
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches Approval Request Comment [Feature/Bug causing the regression]: Bug 1419842 [User impact if declined]: show_matches and hide_matches won't work properly in some cases. They were introduces in Firefox 59, so it would be nice to fix the initial implementation. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: I have manually verifies in nightly [Needs manual test from QE? If yes, steps to reproduce]: See comment 47 [List of other uplifts needed for the feature/fix]: bug 1438274 should be uplifted first. [Is the change risky?]: Not much risky [Why is the change risky/not risky?]: The patch properly handles show_matches and hide_matches in various common situations. Even if I missed some rare edge case, it won't be worse than now. [String changes made/needed]: None
Attachment #8949867 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
Summary: pageAction visibiltiy issues when using show_matches and hide_matches → pageAction visibility issues when using show_matches and hide_matches
Comment 49•6 years ago
|
||
Comment on attachment 8949867 [details] Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches Fix for extension page actions, let's uplift for beta 12.
Attachment #8949867 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 50•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/bc4dc448c43b
Updated•6 years ago
|
Flags: in-testsuite+
Updated•6 years ago
|
Flags: qe-verify+
Comment 51•6 years ago
|
||
I was able to reproduce this issue on Nightly 60.0a1 (2018-02-09) under Windows 10 x64 by following the steps from comment 47. I retested everything on Nightly 60.0a1 (2018-02-25) and Beta 59.0b12 (20180222170353) under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32 and I can confirm this issue is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•