chrome.tabs.onHighlighted undefined

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Goran Sumzer, Assigned: mattw)

Tracking

({dev-doc-complete})

45 Branch
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [tabs]triaged)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151016093648

Steps to reproduce:

called chrome.tabs.onHighlighted from background.js


Actual results:

ReferenceError: reference to undefined property chrome.tabs.onHighlighted
Blocks: 1213477
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [tabs]
Flags: blocking-webextensions?

Updated

2 years ago
Flags: blocking-webextensions? → blocking-webextensions+

Updated

2 years ago
Assignee: nobody → mwein

Updated

2 years ago
Iteration: --- → 47.1 - Feb 8
(Assignee)

Comment 1

2 years ago
Could I get some clarification on how onHighlighted should function? The Firefox Doc says that the event should be "fired when the highlighted or selected tabs in a window changes."  Does this mean that whenever the mouse hovers over a tab it should fire, in addition to it firing whenever a tab is clicked on?
In our case, it should probably just be an alias for onActivated.

Chrome has a concept of highlighted tabs, where you can select multiple tabs to perform an operation on. So, if you click tab 1, hold shift, and then click tab 3, tabs 1-3 become highlighted, and tab 3 becomes active. If you then right click and click "Close Tabs", all three tabs are closed.

Since we don't have a similar concept, we only need to treat the currently active tab as highlighted.
(Assignee)

Comment 3

2 years ago
Created attachment 8713899 [details] [diff] [review]
WIP: implement chrome.tabs.onHighlighted

I tried to write a test for this but I wasn't able to get my highlight listener to fire. I tried |browser.tabs.update(tabs[0].id, { active: true });| which selected the tab but the listener didn't get called.  Do you have some advice on how to write a simple test for this?
<none>
Attachment #8713899 - Flags: review?(kmaglione+bmo)
Comment on attachment 8713899 [details] [diff] [review]
WIP: implement chrome.tabs.onHighlighted

Review of attachment 8713899 [details] [diff] [review]:
-----------------------------------------------------------------

The tests for this are going to be a bit tricky. If there were already tests for onActivated, you could base yours on them... but since there aren't, we should probably add them at the same time.

The easiest thing will probably be to base your tests on the browser_ext_tabs_create.js tests, and possibly a bit on the browser_ext_pageAction_context.js tests.

So, essentially, create a helper function that returns a promise when both the activated and highlighted events have happened, e.g.:

let promiseActivated = (tabId, windowId) => {
  let activatedPromise = new Promise(resolve => {
    let onActivated = details => {
      if (details.tabId === tabId && details.windowId === windowId) {
        browser.tabs.onActivated.removeListener(onActivated);
        browser.test.pass(`Got onActivated event for tab ${tabId}, window ${windowId}`);
        resolve();
      }
    };
    browser.tabs.onActivated.addListener(onActivated);
  });

  let highlightedPromise = new Promise(resolve => {
    let onHighlighted = details => {
      if (details.tabId === tabId && details.windowId === windowId) {
        browser.tabs.onHighlighted.removeListener(onHighlighted);
        browser.test.pass(`Got onHighlighted event for tab ${tabId}, window ${windowId}`);
        resolve();
      }
    };
    browser.tabs.onHighlighted.addListener(onHighlighted);
  });

  return Promise.all([activatedPromise, highlightedPromise]);
};

And then use the tabs API to create several tabs (making sure you get a highlighted and activated event after each create call), and then use browser.tabs.update to change the active tab once or twice.
Attachment #8713899 - Flags: review?(kmaglione+bmo) → feedback+
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40d87fa1dba
(Assignee)

Comment 6

2 years ago
Created attachment 8714927 [details] [diff] [review]
Add support for chrome.tabs.onhighlight
Attachment #8714927 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8714927 [details] [diff] [review]
Add support for chrome.tabs.onhighlight

Review of attachment 8714927 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Thanks.

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js
@@ +60,5 @@
> +          "onActivated",
> +          "onHighlighted",
> +        ]);
> +      }).then(() => {
> +        browser.test.log(`Highlighting tab ${tabIds[0]}`);

Would be good to check that the events are for the right tabs here. And for the events below too.

@@ +88,5 @@
> +          "onHighlighted",
> +        ]);
> +      }).then(() => {
> +        for (let i = 0; i < tabIds.length; i++) {
> +          promiseTabs.remove(tabIds[i]);

We should probably do something like `return Promise.all(tabIds.map(id => promiseTabs.remove(id)))` here, just to be sure the tabs are gone when we end the test. We'll wind up with intermittent failures if they're not.
Attachment #8714927 - Flags: feedback?(kmaglione+bmo) → feedback+
Attachment #8713899 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=718c5600588e
(Assignee)

Comment 9

2 years ago
Created attachment 8715538 [details] [diff] [review]
Add support for chrome.tabs.onhighlight
Attachment #8715538 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8714927 - Attachment is obsolete: true

Updated

2 years ago
Whiteboard: [tabs] → [tabs]triaged

Updated

2 years ago
Priority: -- → P3
Comment on attachment 8715538 [details] [diff] [review]
Add support for chrome.tabs.onhighlight

Review of attachment 8715538 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I thought I'd reviewed this already.

It looks good, just needs a bit of minor cleanup. Thanks.

::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js
@@ +16,5 @@
> +        };
> +      });
> +      return result;
> +    }
> +    let promiseTabs = wrapAPI(browser.tabs);

This isn't necessary anymore. We can just use browser.tabs directly, since they now return promises.

@@ +78,5 @@
> +            "onActivated",
> +            "onHighlighted",
> +          ]);
> +        }).then(() => {
> +          resolve();

No need to use `new Promise` here. This code is equivalent to:

    function openTab(windowId) {
      browser.tabs.create({ windowId }).then(tab => {
        tabIds.push(tab.id);
        browser.test.log(`Opened tab ${tab.id}`);
        return expectEvents(tab.id, [
          "onActivated",
          "onHighlighted",
        ]);
      });
    }

@@ +96,5 @@
> +            "onActivated",
> +            "onHighlighted",
> +          ]);
> +        }).then(() => {
> +          resolve();

Same as above. This is equivalent to:

   function highlightTab(tabId) {
      browser.test.log(`Highlighting tab ${tabId}`);
      return browser.tabs.update(tabId, { active: true }).then(tab => {
        browser.test.assertEq(tab.id, tabId, `Tab ${tab.id} highlighted`);
        return expectEvents(tab.id, [
          "onActivated",
          "onHighlighted",
        ]);
      });
    }
Attachment #8715538 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 11

2 years ago
Created attachment 8716837 [details] [diff] [review]
Add support for chrome.tabs.onHighlighted.
Attachment #8716837 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 12

2 years ago
Thanks for the helpful feedback.
(Assignee)

Updated

2 years ago
Attachment #8715538 - Attachment is obsolete: true
Attachment #8716837 - Flags: review?(kmaglione+bmo) → review+
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
(Assignee)

Updated

2 years ago
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
(Assignee)

Comment 13

2 years ago
Created attachment 8723346 [details] [diff] [review]
Implement chrome.commands.getAll
(Assignee)

Updated

2 years ago
Attachment #8723346 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8716837 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
(In reply to Matthew Wein [:mattw] from comment #13)
> Created attachment 8723346 [details] [diff] [review]
> Implement chrome.commands.getAll

This patch shouldn't have gotten added to this bug. I'm not sure how I mixed this up but I'll resolve it now.
(Assignee)

Comment 15

2 years ago
Created attachment 8723452 [details] [diff] [review]
Add support for chrome.tabs.onhighlight. try: -b do -p linux64,macosx64,win32,win64 -u mochitest,xpcshell -n --post-to-bugzilla
(Assignee)

Comment 16

2 years ago
Created attachment 8723454 [details] [diff] [review]
Add support for chrome.tabs.onHighlighted
(Assignee)

Updated

2 years ago
Attachment #8723346 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723452 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8723454 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/90494d4d76ab
Keywords: checkin-needed

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/764c86198573
Followup push to fix eslint issues: https://hg.mozilla.org/integration/mozilla-inbound/rev/764c86198573
Backed out for e10s browser_ext_tabs_onHighlighted.js permafail.
https://treeherder.mozilla.org/logviewer.html#?job_id=22601997&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/7882aa8df614
Hrm. That error again.

It happens when we wind up with moz-extension: URLs from disabled extensions in about:newtab, and the test machine happens to have its mouse cursor over one of them when the new tab page opens.

I'll take care of it.
Depends on: 1252239
https://hg.mozilla.org/integration/fx-team/rev/57774ff596857a1567ff5e40ce44cbbe2ec8308a
Bug 1220124 - Add support for chrome.tabs.onhighlight. r=kmag

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/57774ff59685
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onHighlighted
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.