Allow extensions to access tab's "attention" flag

RESOLVED FIXED in Firefox 63

Status

P5
enhancement
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: botond, Assigned: arshadkazmi42, Mentored)

Tracking

({dev-doc-complete, good-first-bug})

unspecified
mozilla63
dev-doc-complete, good-first-bug
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Pinned tabs have a feature where, if the tab is in the background and has new activity (e.g. it's an web email client and a new email has arrived), a blue glow appears on top of the pinned tab icon, to alert the user.

It would be good to have an API that allowed an extension to be notified of such new activity as well, so that extensions that aim to replace the tab bar (like Tree Tabs [1]) can provide a similar visual cue.

[1] https://gitlab.com/kroppy/TreeTabs
That "feature" is based on tab title changes, which are already available though the webNaviagation api.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
There's a separate "attention" state for when the tab has a modal dialog showing.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 4

2 years ago
Can someone explain it to me? Is it in changeInfo property? from tabs.onUpdated callback? or is that Tab.status property? like "loading" or "complete"? I don't see it anywhere, it's not present in tab object, neither in changeInfo. There is nothing in documentation about that. Can someone help me with that? Maybe you mean Tab.highlighted property, but it's not triggered from tabs.onUpdated, but perhaps it's another thing?

Updated

2 years ago
status-firefox57: --- → wontfix
Priority: -- → P5
Duplicate of this bug: 1405463
Mentor: bob.silverberg
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Keywords: good-first-bug
Summary: Add an API to let an extension know when a tab has new activity / has been updated → Allow extensions to access tab's "attention" flag

Comment 6

2 years ago
Hi, can I take this up?
> Hi, can I take this up?

Sure, but I need to do a bit of research into exactly what we need to do for this. I'll update the bug as soon as I have that information.(In reply to apoorvasingh2811 from comment #6)
Tbh I'm not convinced this is a good-first-bug, but maybe I am missing an obvious implementation. 

It appears to me that we need to listen for a change to the tab attribute "attention" and then fire an API event when a tab's attention attribute becomes "true". Does that sound accurate? If so, I'm not sure how/where we listen for that.

It also seems possible that we could listen for the DOMWillOpenModalDialog event, although I'm not sure that will always be accurate. If that is what we want to do I have the same question: how/where do we listen for that?
Flags: needinfo?(kmaglione+bmo)

Comment 9

2 years ago
Maybe?
https://dxr.mozilla.org/mozilla-central/rev/11fe0a2895aab26c57bcfe61b3041d7837e954cd/browser/base/content/tabbrowser.xml#6041

Another thing is that you have to expose tab.attention to WebExtensions API, which is not accessible at the moment.
Sorry for the delay.

This should be quite simple to fix. All we need to do is expose the tab's "attention" attribute. Normally, when a tab attribute changes, the tabbrowser code sends a TabAttrModified event, which we handle here:

http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/browser/components/extensions/ext-tabs.js#251-264

I thought we already did this for the "attention" attribute, but apparently we don't. That should be quite easy to fix, though. We just need to add `this._tabAttrModified(tab, ["attention"])` calls in the places where it's changed:

http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/browser/base/content/tabbrowser.xml#6119
http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/browser/base/content/tabbrowser.xml#6132
http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/browser/base/content/tabbrowser.xml#1301

(in the last case, making sure we only send the event if the attribute is already present when we try to remove it).
Flags: needinfo?(kmaglione+bmo)
Mentor: bob.silverberg → rob

Updated

9 months ago
Product: Toolkit → WebExtensions
status-firefox57: wontfix → ---
If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

Mentor: :robwu
(Assignee)

Comment 12

7 months ago
Hi, I want to work on this issue. I am new here. How can I get started? Setup the environment?
Arshad, welcome and thanks for your interest!
If you haven't done it already, building Firefox is a good first step:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

For this bug you should be able to use artifact builds which will save you some time:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

When you're ready to work on the bug, you can post questions here, use the "Need more information from" button below and enter :robwu (the mentor for this bug) to ensure that he notices that you're waiting for a particular question to be answered.

Good luck!
(Assignee)

Comment 14

7 months ago
:aswan thank you for the detailed info.

I am done with the setup of mozilla central project

:robwu 
I am done with the setup of the repo finally :P
But I am not sure where should I start, as mentioned by :kmag as per my understanding I have to modify those files.
Where can I find those files? And how can I run/check the project working in my system?
Flags: needinfo?(rob)

Comment 15

7 months ago
If you have followed :aswan's recommendations in comment 13, you should have a working mozilla-central repository AND dependencies to build. If you don't have the dependencies yet, simply run "./mach bootstrap" from the mozilla-central repository to ensure that the dependencies are installed.

After that, you can build Firefox with the "mach build" command. If you use artifact builds (highly recommended), the build will finish within a few minutes.

If you are familiar with WebExtension extensions already, I suggest that you create a test extension that uses the new API. This enables you to test whether your changes work, without having to learn how Firefox's testing framework works.
You can start Firefox using "mach run" and load the extension at the "about:debugging" page to manually test the extension.

If you want to run a browser test that you developed, run "mach test [path to file or directory]". For example, to run all extension browser tests (that may take a while), run:
mach test browser/components/extensions/test/browser/ toolkit/components/extensions/test/browser/



In comment 10, :kmag showed links to Searchfox, which is an online search engine for the current Firefox code.
The code has changed since. So to find the current location of the code, I copied the phrase 'setAttribute("attention"' from the original code and started a search in Searchfox:
https://searchfox.org/mozilla-central/search?q=setAttribute(%22attention%22%2C&case=false&regexp=false&path=

The results contain two relevant entries:

https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/base/content/tabbrowser.js#4419
https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/base/content/tabbrowser.js#4432

Similarly for: newTab.removeAttribute("attention");
https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/base/content/tabbrowser.js#960

You have to call tabForEvent._tabAttrModified(tab, ["attention"]); after changing the "attention" attribute on the tab.


After making that change, any changes to the "attention" flag will propagate to the implementation of the tabs API:
https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/components/extensions/parent/ext-tabs.js#215-231,253-258

You have to push the "attention" property in the "needed" array when needed, to get the browser.tabs.onUpdated event to fire when the "attention" state changes.


If you managed to make those changes, then you can start with the more challenging part:
- Adding the "attention" flag to the tabs.Tab type (in the scheme and in the implementation).
- Adding "attention" to the accepted filters (allProperties).
- Writing browser tests to verify that the API works as expected.

Here is an example of a recent change that touched many relevant files to add the "sharingState" flag to the tabs API. I suggest that you search for "sharingState" in the commit to discover which files you need to change.
Flags: needinfo?(rob)
(Assignee)

Comment 16

7 months ago
Hi :robwu
I made the first changes to add this
`tabForEvent._tabAttrModified(tab, ["attention"]);` 

below this line
`tabForEvent.setAttribute("attention", "true");`

in tabbrowser.js file

Also I have added "pushing attention flag to needed array" in ext-tabs page

I have not worked with Webextensions before. So can you guide me through on testing this?

How can I search for 'sharingState' in commit is there a place available to search for the commits?
Flags: needinfo?(rob)

Updated

7 months ago
Severity: normal → enhancement

Comment 17

7 months ago
(In reply to Arshad Kazmi from comment #16)
> I have not worked with Webextensions before. So can you guide me through on
> testing this?

Ultimately, automated tests are required to verify that the extension works, so doing manual testing is completely optional.
I would only recommend manual testing if you want to understand the effect of the the change that you are making on actual extensions. If you want to create an extension for manual testing, see
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions#Getting_started

To write an automated browser test, you have to create a new file at browser/components/extensions/test/browser/
and add a line with the test file to browser/components/extensions/test/browser/browser-common.ini
A good name for that new file would be "browser_ext_tabs_attention.js" .
See many of the existing examples at https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/ to learn more about how the tests are structured and what utilities are available.

> How can I search for 'sharingState' in commit is there a place available to
> search for the commits?

In Searchfox, when you hover over the grey bar at the left of the line numbers, the details of the last commit that changed the line shows up.
You can click on "Show annotated diff" to see the diff for that specific file, with all other unchanged lines. For example:
https://searchfox.org/mozilla-central/diff/d78ea2931e7a7c2e44eaf59e31c1d9e6028d5e56/toolkit/components/extensions/ext-tabs-base.js#376

Then click on the link in the upper-left corner "Showing d78ea293: " to see more details about all other changes in the same commit. For example:
https://searchfox.org/mozilla-central/commit/d78ea2931e7a7c2e44eaf59e31c1d9e6028d5e56
From there you can find the links to the diffs, in the "hg" row (and also next to "git"; pick whichever user interface you prefer).
Flags: needinfo?(rob)
(Assignee)

Comment 18

7 months ago
I tried creating a extension following the tutorial.
I am not sure how can I test it. What exactly is this functionality which I have to test for this.
Flags: needinfo?(rob)

Comment 19

7 months ago
This bug is about developing a new feature: Allowing extension to retrieve the "attention" state of a tab, and detecting updates to this state.

To perform a manual test, you need to create an extension that uses the browser.tabs.onUpdated API, for which some examples are available in the documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/onUpdated

When you have created an example, load it via about:debugging as explained at:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Debugging

Then trigger the "attention" state change, for example as follows:
1. Open the developer tools of the tab (F12) and type the following in the console:
   setTimeout(() => { alert("Attention!"); }, 5000);
2. Then switch to a different tab (or open a new tab) and wait 5 seconds. Now the code snippet from the first step will trigger a dialog and cause the tab to draw attention.
3. Check whether your extension's browser.tabs.onUpdated listener has detected the "attention" state change (in the second parameter, documented as changeInfo).
4. Check whether the value of the onUpdated event's third parameter has an "attention" property, set to true.


Again, note that this manual test is only to get you familiar with how an extension works. Once the feature is completely ready, automated browser tests need to be developed (as I explained in comment 17).
Flags: needinfo?(rob)
(Assignee)

Comment 20

7 months ago
Hi Robwu [:robwu]
Thank you for the detailed info. I created a plugin to test it. It seems working fine.

I was trying to write a automated test case, but getting stuck on doing this

`
Open the developer tools of the tab (F12) and type the following in the console:
   setTimeout(() => { alert("Attention!"); }, 5000);
`

using the test script.

Is there any test writing documentation available?
Flags: needinfo?(rob)

Comment 21

7 months ago
In a real test, you should not be using setTimeout because timers with arbitrarily chosen numbers reduce the predictability of a test.
Instead, you should try to navigate a background tab to a test page that displays a modal dialog.

Firefox uses many different test systems for different purposes, an overview is given at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing .
The tests in browser/components/extensions/test/browser/ are briefly documented at https://developer.mozilla.org/en-US/docs/Mozilla/Browser_chrome_tests

Some specifics that only apply to WebExtensions are not documented though, so often the easiest way to learn writing tests is to look at existing tests as examples in https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/

Each test file is listed in browser-common.ini - https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser-common.ini

A test consists of multiple subtasks, which are defined in the test files via add_task(...).
Most tests have the following minimal structure:

add_task(async function someShortAndSimpleDescriptionHere() {
  // Define a test extension.
  let extension = ExtensionTestUtils.loadExtension( .... );

  // Loads the actual extension.
  await extension.startup();

  // Tests here, often using extension.sendMessage and/or extension.awaitMessage

  // Unload the extension and remove the temporary files.
  await extension.unload();
});

ExtensionTestUtils.loadExtension allows you to quickly generate a test extension, and its input parameters are documented at:
https://searchfox.org/mozilla-central/rev/ef8b3886cb173d5534b954b6fb7eb2d94a9473d0/toolkit/components/extensions/ExtensionTestCommon.jsm#185-212


The BrowserTestUtils namespace provides helpful utilities to assist in setting up the test: https://searchfox.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

For example, most tests that involve tabs start the test (i.e. before the ExtensionTestUtils.loadExtension call) with:

  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "... test URL here ... ");
  gBrowser.selectedTab = tab;

and end the test (after extension.unload) with:

  BrowserTestUtils.removeTab(tab);

In your test you likely want to open two tabs, and "about:blank" as the test URL suffices.
Then after the extension is loaded and the extension has registered the event listeners (e.g. browser.tabs.onUpdated), you can navigate a background tab (e.g. using BrowserTestUtils.loadURI) to a test page that shows a modal dialog. Since this is a very simple page, you could use the following URL for testing purposes:
data:text/html;charset=utf-8,<script>alert("Attention")</script>
Flags: needinfo?(rob)
(Assignee)

Comment 22

7 months ago
Hi :robwu
I tried working on the automated test, but couldn't figure it out exactly how it works.

I have written test something like this and I am not sure its right or not

`
/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
/* vim: set sts=2 sw=2 et tw=80: */
"use strict";

add_task(async function testTabAttention() {
  const alertUrl = `data:text/html;charset=utf-8,<script>setTimeout(function(){ alert('This is an alert'); }, 100)</script>`
  let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, alertUrl);
  let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, `about:blank?2`)
  gBrowser.selectedTab = tab2;
  let extension = ExtensionTestUtils.loadExtension({
    manifest: {
      "permissions": ["tabs"],
    },

    background: async function() {     

      try {
        browser.tabs.query({active: true, currentWindow: true}, tabs => {
          activeTab = tabs[0];
          browser.test.assertEq(alertUrl, activeTab.url, `Expected value for tab .${activeTab.url}`);
          browser.test.notifyPass("tabsAttention");
        });

      } catch (e) {
        browser.test.fail(`Error: ${e} :: ${e.stack}`);
        browser.test.notifyFail("tabsAttention");
      }
    },
  });

  await extension.startup();

  await extension.awaitFinish("tabsAttention");

  await extension.unload();
  BrowserTestUtils.removeTab(tab1)
  BrowserTestUtils.removeTab(tab2)
});
`

I think I need more help with the automated test.
Flags: needinfo?(rob)

Comment 23

7 months ago
Your test looks syntactically correct. Tests should generally be written under the assumption that the test passes, so you don't need to use try-catch.
Moreover, the async syntax allows you to write the test in a shorter way, like this:

background: async function() {
  let [activeTab] = await browser.tabs.query({active: true, currentWindow: true});
browser.test.assertEq(alertUrl, activeTab.url, `Expected value for tab .${activeTab.url}`);
browser.test.notifyPass("tabsAttention");
}

What part of your test is not working? If you created a new file, did you add it to browser-common.ini ?

And at the start of your test, tab1 and tab2 are opened, followed by marking tab2 as the selected tab.
The active tab is therefore tab2. But the test then asserts that the active tab's URL is alertUrl. Since alertUrl belongs to tab1, and the active tab is tab2, the test will fail.

PS. Consider joining our IRC channel, #webextensions for real-time conversations. If you have never used IRC before, see https://wiki.mozilla.org/IRC . It is easy to use, e.g. via Mibbit.
Flags: needinfo?(rob)
(Assignee)

Comment 24

7 months ago
So after executing the test, I am getting this error

FAIL uncaught exception - ReferenceError: tab is not defined at _setupEventListeners/<@chrome://browser/content/tabbrowser.js:4421:11

when I set timeout to 500.

Basically, I am trying to open two tabs, first tab will show a alert after 500ms and setting the second tab as selected. so after 500ms when I match the active tab it should be first tab, as alert will activate attention flag.

Is my above logic fine to test this feature & also is my test correct as per my logic?
Flags: needinfo?(rob)

Comment 25

7 months ago
The error suggests that there is an actual error in your implementation in tabbrowser.js
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ReferenceError
and review your own code again.

You should not use "setTimeout" in tests because the test may become non-deterministic (https://en.wikipedia.org/wiki/Nondeterministic_algorithm), and cause the test to fail unexpectedly when a time-sensitive parameter changes.
For instance, if the test runs very slowly, and it takes one second to open the second tab, then the alert dialog will appear before the tab switches, and the "attention" flag would not be set.

To avoid this issue without using timers, open two tabs first, and then navigate the inactive tab to a page that shows an alert dialog. See the end of comment 21 for more details.

Your logic is almost correct. The only thing to note is that the "attention" flag is expected at the inactive tab, not the active tab. Instead of selecting the active tab in the current window, you could try to select the tab with the expected URL, e.g.
let [tab] = await browser.tabs.query({url: "data:*alert*"});
// TODO: Assert the attention state of the tab.

You can keep the existing code that retrieved the current tab, and assert that its attention property is false.
Flags: needinfo?(rob)
(Assignee)

Comment 26

7 months ago
So, I was trying to do assert on attention state of the tab. But after some debugging I found, attention state is not getting set to the tab. 
Is there any other change required in the main code. I have added 

this._tabAttrModified(tab, ["attention"]);

Is there any other change required for this to work.
Flags: needinfo?(rob)

Comment 27

7 months ago
Pause for a moment, and think about what that line is expected to do.
Then look at the error message (the ReferenceError, see my MDN documentation link if you don't know its meaning) and then look at the code again (especially in relation with the other lines around your code).
Then you will probably see what needs to be done.

This kind of error can also be caught by linting (via eslint). E.g. via the following command:
mach lint browser/base/content/tabbrowser.js
Flags: needinfo?(rob)
(Assignee)

Comment 28

7 months ago
i have fixed the lint issues using mach lint

I have one doubt, so how can I getch 'tab' object at this position

https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/base/content/tabbrowser.js#4419
https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/base/content/tabbrowser.js#4432

Currently i am using this

`let tab = this._getTabForContentWindow(event.target);`

and in this line

https://searchfox.org/mozilla-central/rev/246f2b4fab2c1a6cca99418bc2e4d73d1102cc38/browser/base/content/tabbrowser.js#960

i have used this

this._tabAttrModified(newTab, ["attention"]);
this._tabAttrModified(oldTab, ["attention"]);

Is something wrong with this?
Flags: needinfo?(rob)

Comment 29

7 months ago
If unsure about the expected parameter format, just look up the implementation of the function that you're calling.
https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/browser/base/content/tabbrowser.js#1171-1183

If you're familiar with JavaScript/DOM, then you will immediately know that the tab parameter is expected to be a DOM element.
When you look at the relevant code in tabbrowser.js, then you could guess that tabForEvent is a DOM element, because it supports the setAttribute method.

So what you can do is using the "tabForEvent" as the parameter.

Another way to see this is by considering what you're doing.
You are trying to trigger an event when the "attention" state changes.
Naturally, this event has to be dispatched on the object whose attribute changed, i.e. tabForEvent.

(In reply to Arshad Kazmi from comment #28)
> i have used this
> 
> this._tabAttrModified(newTab, ["attention"]);
> this._tabAttrModified(oldTab, ["attention"]);
> 
> Is something wrong with this?

I will leave answering this question as an exercise to you (hint: try to use the same reasoning as I showed above).
Flags: needinfo?(rob)
(Assignee)

Comment 30

7 months ago
I went through the function and modified the codes

`this._tabAttrModified(tabForEvent, ["attention"]);`

&& 

like this, for second file

`this._tabAttrModified(newTab, ["attention"]);`

But still attention flag is not getting set to the tab.
I event trying triggering sendMessage, executeQuery events on tab. But still attention flag is not getting set
Flags: needinfo?(rob)

Comment 31

7 months ago
Can you show your full patch? Perhaps you've forgot to add some necessary lines to your code.

To submit code for review, we use Phabricator, which is documented at:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
Assignee: nobody → arshadkazmi42
Status: REOPENED → ASSIGNED
Flags: needinfo?(rob)
(Assignee)

Comment 32

7 months ago
Hi, I have added my code to Phabricator for review

https://phabricator.services.mozilla.com/D3984
Flags: needinfo?(rob)

Comment 33

7 months ago
With your changes so far, you have only managed to copy the "attention" value of the tab to changeInfo (=second parameter of tabs.onUpdated event) whenever the tab changes, because you are appending "attention" to the `needed` list.
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/browser/components/extensions/parent/ext-tabs.js#253-256

However, since you have not made any other changes, `tab.attention` is `undefined`, and therefore you would be assigning the `undefined` value to `changeInfo`

You need to add more code to make sure that the "attention" property is set on the tab object.
The full list of files and lines that you need to change (=add the "attention" flag, string or implementation depending on the code) is:

https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/browser/components/extensions/parent/ext-tabs.js#125-137
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/browser/components/extensions/parent/ext-tabs.js#215-231 (already done)
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/toolkit/components/extensions/parent/ext-tabs-base.js#474-484
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/toolkit/components/extensions/parent/ext-tabs-base.js#578-598
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/mobile/android/components/extensions/ext-utils.js#558-570
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/browser/components/extensions/parent/ext-browser.js#781-788
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/browser/components/extensions/schemas/tabs.json#76-104
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/mobile/android/components/extensions/schemas/tabs.json#75-103

When you add new code, please be consistent and choose approximately the same location for the new code.


If you want to practice test-driven development (highly recommended), I suggest that you first write a test that checks that the "attention" flag is present or not. That test should fail now, because the "attention" property is of course not implemented yet.
After making all other above changes, you can verify that the feature is working as intended by the passing test.
Flags: needinfo?(rob)
(Assignee)

Comment 34

7 months ago
I did the above changes. But now this function is returning undefined value for attention
in this page `browser/components/parent/extensions`
`
 get attention() {
   return this.nativeTab.attention;
 }
`

Am fetching it wrong? Looks like its not getting set into nativeTab.

I event tried doing something like this

this.nativeTab.getAttribute("attention")

but this also returns undefined.
Flags: needinfo?(rob)

Comment 35

7 months ago
"attention" is an attribute that is only set when... ...the "attention" attribute is set.

When you use .getAttribute("attention"), there are two possible states:
null - when the attribute is not set (or removed via .removeAttribute("attention");
"true" - when the attribute is set (via .setAttribute("attention", "true");


The only reason that other properties such as "this.nativeTab.hidden" work is because there is an implementation of the property getter:
https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/browser/base/content/tabbrowser.xml#1767-1771
(with this additional example, it should now be obvious how you can implement the "attention" getter).
Flags: needinfo?(rob)
(Assignee)

Comment 36

7 months ago
I have added this property in the tabbrowser.xml file

 <property name="attention" readonly="true">
        <getter>
          return this.getAttribute("attention") == "true";
        </getter>
      </property>


below this 

 <field name="_selectedOnFirstMouseDown">false</field>


Now its always returning attention: false,

I think tabs attention attribute is not getting set.
or am I doing something wrong? I tried running alert from browser console and also trigger update event on tab 0 keeping tab 2 as selected
Flags: needinfo?(rob)

Comment 37

7 months ago
(In reply to Arshad Kazmi from comment #36)
> I have added this property in the tabbrowser.xml file
> 
>  <property name="attention" readonly="true">
>         <getter>
>           return this.getAttribute("attention") == "true";
>         </getter>
>       </property>
> 
> 
> below this 
> 
>  <field name="_selectedOnFirstMouseDown">false</field>

Since the property is not used anywhere except in this patch, I suggest to not add a new property and instead use this logic directly in your ext-browser.js

> Now its always returning attention: false,
> 
> I think tabs attention attribute is not getting set.
> or am I doing something wrong? I tried running alert from browser console
> and also trigger update event on tab 0 keeping tab 2 as selected

The "attention" attribute is only set when a dialog is shown in a background tab.
If you show an "alert" dialog in the current tab, the "attention" attribute is not set.
Flags: needinfo?(rob)
(Assignee)

Comment 38

7 months ago
What exactly is a background tab?

So if there are four tabs open in a browser, other than the active tab which is getting used all other tabs are background tabs??
Flags: needinfo?(rob)

Comment 39

7 months ago
Yes.
Flags: needinfo?(rob)
(Assignee)

Comment 40

7 months ago
Got it working :)
Attention flag is getting set.

One more thing. is it possible to show a alert from test script?
I tried loading this as url for a tab 

`data:text/html;charset=utf-8,<script>alert("Attention")</script>`

but it throws error saying illelag url.

Is there a way to open alert box from browser.tabs event
Flags: needinfo?(rob)

Comment 41

7 months ago
That URL should work. How are you trying to open the URL?
Flags: needinfo?(rob)
(Assignee)

Comment 42

7 months ago
This is how i am loading this url in tab

`await browser.tabs.update(tab.id, {
   url: `data:text/html;charset=utf-8,<script>alert("Attention")</script>`,
 });`


this is the error I am getting

Error: Illegal URL: data:text/html;charset=utf-8,<script>alert("Attention")</script>
Flags: needinfo?(rob)

Comment 43

7 months ago
The browser.tabs API does not support loading data:-URLs in tabs.

You have to use BrowserTestUtils.loadURI to navigate an existing tab. Look at some of the existing examples for inspiration:
https://searchfox.org/mozilla-central/search?q=BrowserTestUtils.loadURI&case=false&regexp=false&path=extensions%2Ftest
Flags: needinfo?(rob)
(Assignee)

Comment 44

7 months ago
I was trying to do something like this.


add_task(async function testPrintPreview() {
  await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.org");
  await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");

  let extension = ExtensionTestUtils.loadExtension({
    manifest: {
      "permissions": ["tabs"],
    },

    background: async function() {
      const tab = await browser.tabs.query({active: false});
      await browser.tabs.update(tab.id, {
        url: `data:text/html;charset=utf-8,<script>alert("Attention")</script>`,
      });
      browser.test.assertEq(true, tab.attention, "tab attention set");
      browser.test.notifyPass("tabsAttention");
    },
  });

  await extension.startup();
  await extension.awaitFinish("tabsAttention");
  await extension.unload();

  BrowserTestUtils.removeTab(gBrowser.tabs[0]);
  BrowserTestUtils.removeTab(gBrowser.tabs[1]);
});


I have already checked other test cases for similar scenarios, but i want to load alert url from extenion so it will load in the background tab and then i can very "attention" flag in the same tab.

BrowserTestUtils is not accessible in "background" function
Flags: needinfo?(rob)

Comment 45

7 months ago
Firstly, browser.tabs.query returns the state of the tab at the time time of the invocation. When you navigate the tab, the returned "tab" object will not change, and in particular its tab.attention value will still be false.

Secondly, you can use browser.test.sendMessage inside the test to notify the outer part of the test, and then use extension.sendMessage from the outer part of the test to notify the background script of the test extension:
When you call extension.sendMessage from the outer part of the test, the browser.test.onMessage event is fired in the background script of the test extension.

Here is an example of a test that uses the tabs events and goes back and forths multiple times between the background script and the test driver:
https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/browser/components/extensions/test/browser/browser_ext_tabs_events.js#173-247

In case you don't know: The ExtensionTestUtils.loadExtension method serializes the function (turns it into a string) and then creates a background page with that string as the script text. So whatever you put in "background" will run in the test extension's background page, and not have access to the variables or APIs from the parent scope. That is why you cannot use BrowserTestUtils in the "background" "function".
Flags: needinfo?(rob)
(Assignee)

Comment 46

7 months ago
So I reached to this point but, now I am facing issue with BrowserTestUtils.loadURI needs a browser to load url. But i need to load the url in a tab. below is my test script.
Can you help me out, how can I load the data url in the tab


add_task(async function tabsAttention() {
  await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.org", true);
  await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/", true);

  let extension = ExtensionTestUtils.loadExtension({
    manifest: {
      "permissions": ["tabs"],
    },

    background: async function() {
      browser.tabs.onUpdated.addListener(function listener(tabId, changed, tab) {
        browser.test.assertEq("ListnerActive", "ListnerActive", "Reached Listener");
        browser.test.assertEq(true, tab.attention, "Tab has attention stat");
      });

      let tab = await browser.tabs.query({index: 0});
      browser.test.assertEq("true", JSON.stringify(tab[0]), "Checking tab");
      browser.test.sendMessage("load", tab[0]);

      browser.test.sendMessage("tabsAttention");
    },
  });

  extension.onMessage("load", async (tab) => {
    await BrowserTestUtils.loadURI(gBrowser, `data:text/html;charset=utf-8,<script>alert("Attention")</script>`);
  });

  await extension.startup();

  await extension.awaitFinish("tabsAttention");
  await extension.unload();

  BrowserTestUtils.removeTab(gBrowser.tabs[0]);
  BrowserTestUtils.removeTab(gBrowser.tabs[1]);
Flags: needinfo?(rob)
(Assignee)

Comment 47

7 months ago
Hey :robwu

I am done with the test script and the patch.
Test case is passing :)

How can i submit my code for review?

Comment 48

7 months ago
You can submit a patch to Phabricator.

You have already set up an account before using:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Now use rhe "arc" tool to submit the code for review (and to update your revision after code review, if necessary). The arc tool can be installed using these instructions:
https://phabricator.services.mozilla.com/book/phabricator/article/arcanist_quick_start/

To submit a patch, see:
https://moz-conduit.readthedocs.io/en/latest/arcanist-user.html
Flags: needinfo?(rob)
(Assignee)

Comment 49

7 months ago
Add code to Phabricator for review

https://phabricator.services.mozilla.com/D3984

Do I need to mention reviewer also?
Flags: needinfo?(rob)
(Assignee)

Comment 50

7 months ago
Added you as reviewer for my code in Phrabricator. Let me know if I need to add anyone else.
Attachment #9003974 - Attachment description: tab attention feature patch → Bug 1396684 - Allow extensions to access tab's "attention" flag
(Assignee)

Comment 52

7 months ago
Comment on attachment 9003974 [details]
Bug 1396684 - Allow extensions to access tab's "attention" flag

Just a reminder for review of the patch.

Also do I have to mark someone else for as reviewer?
Attachment #9003974 - Flags: review+

Comment 53

7 months ago
Comment on attachment 9003974 [details]
Bug 1396684 - Allow extensions to access tab's "attention" flag

I am currently reviewing your patch, and will post a review shortly.

r+ means that a patch has been reviewed. Since that is not the case, please do not manually add the r+ flag to the patch.
Flags: needinfo?(rob)
Attachment #9003974 - Flags: review+
(Assignee)

Comment 54

7 months ago
Ok. Thanks for the info. 
I thought r+ is for reminding for review.

Comment 55

7 months ago
Comment on attachment 9003974 [details]
Bug 1396684 - Allow extensions to access tab's "attention" flag

Rob Wu [:robwu] has approved the revision.
Attachment #9003974 - Flags: review+
Comment on attachment 9003974 [details]
Bug 1396684 - Allow extensions to access tab's "attention" flag

Shane Caraveo (:mixedpuppy) has approved the revision.
Attachment #9003974 - Flags: review+

Comment 57

7 months ago
Hi Arshad,

Congratulations on having your first patch approved!
When the patch is ready to land (i.e. you have verified that all tests pass), edit the bug and add the checkin-needed keyword.
(Assignee)

Comment 58

7 months ago
Hi Rob,

Thanks you for helping during the work on patch.
I didn't get the last line 
"you have verified that all tests pass"

I have already written an automation test to test this feature.

Do I have to do something else for more testing? 
Or I can add the checkin-needed keyword?
Flags: needinfo?(rob)

Comment 59

7 months ago
During development, you have created a new test that verifies that the new behavior works as expected.
To make sure that you did not inadvertently break other things, you can run existing unit tests and confirm that they pass.

I have started a test on the try server for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c4b13cbca5fea01ff5b18415ac9c24d97dc4681

If all tests pass (and I expect they will, maybe in an hour or so), then you can add the checkin-needed label.
Flags: needinfo?(rob)
(Assignee)

Comment 60

7 months ago
ok. thank you.

So, just for more information, for every patch I complete.
I have to run the tests on the try server once its reviewed?
Also do I have to do it, or I have to request someone for it?
Flags: needinfo?(rob)

Comment 61

7 months ago
You cannot use the try server, but you can also run tests locally via mach test (see comment 15).

It is also possible to request checkin without running all tests if you are confident that the patches won't break any existing logic. If things break anyway, the patch will be backed out. We usually try to avoid this because it is inconvenient for everyone involved.
Flags: needinfo?(rob)
(Assignee)

Comment 62

7 months ago
Hi Rob,
The tests on try server is completed. Its showing 1 failed tests and I am not sure what exactly it is.
Can you check it once let me know, if something is breaking due to my changes?
Flags: needinfo?(rob)

Comment 63

7 months ago
That red failured looks unrelated to your patch.
None of the other orange failures look related to your patch either.

You're good to go :)
Flags: needinfo?(rob)
(Assignee)

Comment 64

7 months ago
Awesome. Thank you for all the help throughout the process.
(Assignee)

Comment 65

7 months ago
Hey Rob,
So do I need to update check in needed flag here?
Flags: needinfo?(rob)

Comment 66

7 months ago
At the top of this bug page, click on "Edit bug", and append checkin-needed to the keywords field.
Currently the field contains good-first-bug, so the result should be: good-first-bug,checkin-needed
Flags: needinfo?(rob)
(Assignee)

Updated

7 months ago
Keywords: checkin-needed
(Assignee)

Comment 67

7 months ago
Thanks for the info Rob.
I appended check-in needed keyword.

Comment 68

7 months ago
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a1020606032
Allow extensions to access tab's "attention" flag r=mixedpuppy,robwu
Keywords: checkin-needed

Comment 69

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a1020606032
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

7 months ago
Keywords: dev-doc-needed

Comment 70

7 months ago
As I understand from previous comments, automated tests have passed, please set "qe-verify -" flag if manual testing will not be required on this bug.

Thank you!
Flags: needinfo?(arshadkazmi42)
(Assignee)

Updated

7 months ago
Flags: needinfo?(arshadkazmi42) → qe-verify-
(Assignee)

Comment 71

7 months ago
Added qe-verify - flag
(Assignee)

Comment 72

6 months ago
Just needed to check on this bug.
I have added qe-verify - flag
But as per discussion on other bugs, I am not so sure about it?

@Rob: Do we need manual testing on this?

Should I create a test addon for this also for manual testing?
Flags: needinfo?(rob)

Comment 73

6 months ago
Your automated tests have covered the relevant scenarios, so no manual testing is necessary.
Flags: needinfo?(rob)
(Assignee)

Comment 74

6 months ago
ok. thanks for the clarification. :)

Comment 75

6 months ago
Added a description of the new property to the changeInfo object and to the tabs.Tab object as follows:

attention

boolean. Indicates whether the tab is drawing attention. For example, when a new email message arrives, and the tab draws the user's attention by adding a glow to the tab, attention will be true.

Also added to the release notes:

The tabs.onUpdated event can be used to track when a tab is drawing the user's attention with attention property of the changeInfo object (bug 1396684).
Flags: needinfo?(arshadkazmi42)
Keywords: dev-doc-needed → dev-doc-complete

Comment 76

6 months ago
> For example, when a new email message arrives, and the tab draws the user's attention by adding a glow to the tab, attention will be true.

This description applies to attention-drawing by title changes (comment 1 and comment 2).

The feature implemented in this bug is about attention-drawing by modal dialogs (comment 3).
So please update the description to: "For example, when a modal dialog is shown by the page in the tab."
Flags: needinfo?(arshadkazmi42)

Comment 77

6 months ago
(In reply to Rob Wu [:robwu] from comment #76)
> > For example, when a new email message arrives, and the tab draws the user's attention by adding a glow to the tab, attention will be true.
> 
> This description applies to attention-drawing by title changes (comment 1
> and comment 2).
> 
> The feature implemented in this bug is about attention-drawing by modal
> dialogs (comment 3).
> So please update the description to: "For example, when a modal dialog is
> shown by the page in the tab."

Made the change.
You need to log in before you can comment on or make changes to this bug.