chrome.tabs.onHighlighted undefined

RESOLVED FIXED in Firefox 47

Status

defect
P3
normal
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: surmazter, Assigned: mattw)

Tracking

({dev-doc-complete})

45 Branch
mozilla47
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [tabs]triaged)

Attachments

(1 attachment, 6 obsolete attachments)

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [tabs]
Flags: blocking-webextensions?
Flags: blocking-webextensions? → blocking-webextensions+
Assignee: nobody → mwein
Iteration: --- → 47.1 - Feb 8
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.
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+
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
Attachment #8715538 - Flags: review?(kmaglione+bmo)
Attachment #8714927 - Attachment is obsolete: true
Whiteboard: [tabs] → [tabs]triaged
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)
Attachment #8716837 - Flags: review?(kmaglione+bmo)
Thanks for the helpful feedback.
Attachment #8715538 - Attachment is obsolete: true
Attachment #8716837 - Flags: review?(kmaglione+bmo) → review+
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
Attachment #8723346 - Flags: review+
Attachment #8716837 - Attachment is obsolete: true
(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.
Attachment #8723346 - Attachment is obsolete: true
Attachment #8723452 - Attachment is obsolete: true
Attachment #8723454 - Flags: review+
Keywords: checkin-needed
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/mozilla-central/rev/57774ff59685
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.