Last Comment Bug 734835 - Failure in testBackgroundTabScrolling | Right scroll arrow has been highlighted
: Failure in testBackgroundTabScrolling | Right scroll arrow has been highlighted
Status: RESOLVED FIXED
[mozmill-test-failure]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Henrik Skupin (:whimboo)
:
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on: 734273 734814 804561
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 03:23 PDT by Remus Pop (:RemusPop)
Modified: 2012-10-23 20:49 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed
fixed
fixed
unaffected


Attachments
skip test (beta) (2.31 KB, patch)
2012-03-15 02:50 PDT, Remus Pop (:RemusPop)
vlad.mozbugs: review-
Details | Diff | Splinter Review
fix patch v1.0 (2.32 KB, patch)
2012-05-16 04:05 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v2.0 (2.77 KB, patch)
2012-06-15 02:04 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v3.0 (2.68 KB, patch)
2012-06-18 01:11 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v4.0 (2.72 KB, patch)
2012-06-19 08:14 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
Patch v5 (2.37 KB, patch)
2012-07-04 04:54 PDT, Henrik Skupin (:whimboo)
dave.hunt: review+
Details | Diff | Splinter Review
Patch v5.1 (2.26 KB, patch)
2012-07-04 07:18 PDT, Henrik Skupin (:whimboo)
dave.hunt: review+
Details | Diff | Splinter Review

Description Remus Pop (:RemusPop) 2012-03-12 03:23:32 PDT
The test fails only on Linux since 03.08.
This is reproducible and I can see that it stops opening tabs for some reason.
Comment 1 Henrik Skupin (:whimboo) 2012-03-12 03:50:35 PDT
Is it a regression? ALso what do you mean with 03.08? I assume 8th of March?
Comment 2 Remus Pop (:RemusPop) 2012-03-12 03:52:48 PDT
Yes, that's right, it's 8th of March. 
I'm looking into changes merged to trunk from 7th to 8th of March to see if I can get any luck.
It's strange that it fails only on Linux.
Comment 3 Remus Pop (:RemusPop) 2012-03-12 07:39:29 PDT
I think bug 734814 will have the fix for this one as well.
Linux has a different behaviour of tabs.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-12 11:40:22 PDT
If this is failing due to an intended change in Firefox, lets disable this test until bug 734814 is fix. If this is failing due to a bug in Firefox, we need to file a Firefox bug. This should be standard procedure (as per bug 734814 comment 15).
Comment 5 Henrik Skupin (:whimboo) 2012-03-13 01:35:55 PDT
Remus, does the right arrow flash visually? I mean it could be that we do not catch the internal state of it because it's too quick. Thing is that we do not use an event listener yet.
Comment 6 Remus Pop (:RemusPop) 2012-03-13 02:19:23 PDT
It doesn't flash because not enough tabs are open. I think bug 734273 might be the issue here.
I've seen in Aurora on linux that the arrows appear with few tabs open.

An Aurora build with the fix is not out yet, but we should wait for it because this is a regression.
Comment 7 Remus Pop (:RemusPop) 2012-03-13 05:32:37 PDT
I can confirm this is fixed on Aurora as well, using the latest build.
Comment 8 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-13 12:23:48 PDT
Verified that this test has stopped failing after March 12th.
Comment 9 Henrik Skupin (:whimboo) 2012-03-13 22:47:53 PDT
It's still failing in my CI for a ja build on Aurora:
http://mozmill-ci.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445b27c9f
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-14 14:40:52 PDT
Also failing in 12.0 Beta today:
http://mozmill-release.blargon7.com/#/functional/report/e438d6e3916b2b636037d77445ce6459

Remus, if this is a Firefox bug, please open a Firefox bug. If it's a bug in the test please disable the test until you can come up with a fix.
Comment 11 Remus Pop (:RemusPop) 2012-03-15 02:50:06 PDT
Created attachment 606152 [details] [diff] [review]
skip test (beta)

I hate doing this but I cannot reproduce locally so it's pretty hard to fix.
Comment 12 Maniac Vlad Florin (:vladmaniac) 2012-03-15 03:12:33 PDT
Comment on attachment 606152 [details] [diff] [review]
skip test (beta)


> 
>-/**
>- * Map test functions to litmus tests
>- */
>-// testOpenInBackgroundTab.meta = {litmusids : [8259]};

Why are you removing this code block?
Comment 13 Remus Pop (:RemusPop) 2012-03-15 03:29:20 PDT
AFAIK, we no longer use comments regarding the litmus ID, so it's safe to remove them, but I may be wrong. Henrik could you please help?

Also, just saw that FIREFOX_12_0b1_BUILD1 with changeset 249ecd7beaf3 is now a close tree. We'll have to wait for new results from _BUILD2, so I'm canceling skipping until new data arrives.
Comment 14 Remus Pop (:RemusPop) 2012-03-15 03:31:47 PDT
AFAIK, we no longer use comments regarding the litmus ID, so it's safe to remove them, but I may be wrong. Henrik could you please help?
Comment 15 Remus Pop (:RemusPop) 2012-03-15 03:31:54 PDT
AFAIK, we no longer use comments regarding the litmus ID, so it's safe to remove them, but I may be wrong. Henrik could you please help?

Also, just saw that FIREFOX_12_0b1_BUILD1 with changeset 249ecd7beaf3 is now a close tree. We'll have to wait for new rseult from _BUILD2, so I'm canceling skipping until new data arrives.
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-26 13:54:19 PDT
(In reply to Remus Pop (:RemusPop) from comment #13)
> AFAIK, we no longer use comments regarding the litmus ID, so it's safe to
> remove them, but I may be wrong.

It has nothing to do with fixing this test. Only alter, add, or remove code which serves to fix the failure.

> Also, just saw that FIREFOX_12_0b1_BUILD1 with changeset 249ecd7beaf3 is now
> a close tree. We'll have to wait for new results from _BUILD2, so I'm
> canceling skipping until new data arrives.

Why does this matter? If this is a reproducible bug in Firefox 12.0b2 then a bug should be filed and this test should remain active so as to show the true failure. If this is a problem with the test then it matters not and should be disabled/fixed.
Comment 17 Remus Pop (:RemusPop) 2012-03-27 00:09:34 PDT
I no longer see this failing.
Comment 18 Henrik Skupin (:whimboo) 2012-03-27 00:18:31 PDT
If it was an exiting failure it can't be invalid. Changing to WFM.
Comment 19 Remus Pop (:RemusPop) 2012-05-15 01:06:02 PDT
This is reoccurring, but this time on OSX too, besides Vista. Linux is unaffected.
Comment 20 Henrik Skupin (:whimboo) 2012-05-15 01:11:07 PDT
It's somewhat intermittent. Vlad, can you please investigate this failure?
Comment 21 Maniac Vlad Florin (:vladmaniac) 2012-05-15 04:25:38 PDT
(In reply to Henrik Skupin (:whimboo) from comment #20)
> It's somewhat intermittent. Vlad, can you please investigate this failure?

On it
Comment 22 Maniac Vlad Florin (:vladmaniac) 2012-05-15 04:48:35 PDT
I am seeing the following error on Mac OS X 10.7 

ERROR | Test Failure: {"exception": {"stack": "TimeoutError@resource://mozmill/modules/utils.js:447\nwaitFor@resource://mozmill/modules/utils.js:485\n@resource://mozmill/modules/controller.js:685\n@resource://mozmill/modules/frame.js -> file:///Users/svuser/Desktop/fixFailureBackgroundTabScrolling/mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js:71\n@resource://mozmill/modules/frame.js:563\n@resource://mozmill/modules/frame.js:632\n@resource://mozmill/modules/frame.js:675\n@resource://mozmill/modules/frame.js:512\n@resource://mozmill/modules/frame.js:687\n@resource://jsbridge/modules/server.js:184\n@resource://jsbridge/modules/server.js:188\n@resource://jsbridge/modules/server.js:288\n", "message": "Right scroll arrow has been highlighted", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 447}}

1/3 runs the test fails with a timeout error so we are probably not waiting enough there. Moving on further
Comment 23 Henrik Skupin (:whimboo) 2012-05-15 05:00:32 PDT
Or we miss that property change due to our delay in checking it. Probably we should listen for an event?
Comment 24 Maniac Vlad Florin (:vladmaniac) 2012-05-15 05:06:31 PDT
(In reply to Henrik Skupin (:whimboo) from comment #23)
> Or we miss that property change due to our delay in checking it. Probably we
> should listen for an event?

Will confirm that once I am certain. In both ways I would also recommend listening for an event
Comment 25 Maniac Vlad Florin (:vladmaniac) 2012-05-15 07:59:09 PDT
Well its certain alright...Henrik your assumption was right. We miss to check the property on time. 

The timeout for the property removal is set to 150 (see tabbrowser.xml to confirm) 

But adding an event listener will do the exact same thing as the .getNode().hasAttribute('notifybgtab') check we are doing. 

Suggestions are welcomed, guys
Comment 26 Henrik Skupin (:whimboo) 2012-05-15 08:31:11 PDT
Well, right now we are polling for a change of the value, right? With an event listener for the overflow event we will get notified. It would be similar to the window close code we had before in utils.js.
Comment 27 Maniac Vlad Florin (:vladmaniac) 2012-05-16 04:05:52 PDT
Created attachment 624338 [details] [diff] [review]
fix patch v1.0

Fix patch v1.0 

* Added an event listener for the "DOMAttrModified" event. The only attribute which is modified for our object is the scroll button animation attribute 
* Skip patch is made obsolete because we have a fix now
Comment 29 Henrik Skupin (:whimboo) 2012-05-17 13:15:53 PDT
Comment on attachment 624338 [details] [diff] [review]
fix patch v1.0

>+  // Add an event listener to catch scroll button animation
>+  scrollButtonDown.getNode().addEventListener("DOMAttrModified", function () {
>+    scrollButtonDownAnimation = true;
>+  }, false);

We do not want to add a listener for DOMAttrModified. Please see:
https://developer.mozilla.org/en/Extensions/Performance_best_practices_in_extensions#Avoid_DOM_mutation_event_listeners

Instead there should be an overflow event. You can ask Dao or check MXR if you need more information. I don't have that either at the moment. Also never forget to remove an event listener because that would cause leakage.
Comment 30 Maniac Vlad Florin (:vladmaniac) 2012-05-18 00:12:07 PDT
You are so right

Well the "DOMAttrModified" was the only thing I could find to make this actually work at that time, I was somehow aware that its not that reliable :( 
Lets see if we can find a better event to listen for
Comment 31 Henrik Skupin (:whimboo) 2012-05-23 09:53:57 PDT
Any update here? We kinda would like to see this intermittent failure fixed.
Comment 32 Maniac Vlad Florin (:vladmaniac) 2012-05-23 11:57:15 PDT
(In reply to Henrik Skupin (:whimboo) from comment #31)
> Any update here? We kinda would like to see this intermittent failure fixed.

I did lots of trying with the event you've recommended and found out that the overflow event is not something we want because its an event for tabstrip overflow and not for our case with the right scroll button animation. we would have to listen for that event and do the exact same check we did before..id does not help. 
I wanted to find some other solution to this case but no luck so far
Comment 33 Henrik Skupin (:whimboo) 2012-05-24 16:24:13 PDT
Vlad, please ask Dao as already mentioned in comment 29. I'm sure he can help here.
Comment 34 Maniac Vlad Florin (:vladmaniac) 2012-05-28 01:37:55 PDT
cc-ing Dao 

Dao, do we have an event for the right scrolling arrow animation on the tabstrip? 
We need to listen for that event in this test, and you'd be a great help if you can offer me some guidance. Lots of thanks!
Comment 35 Henrik Skupin (:whimboo) 2012-06-05 04:19:03 PDT
Vlad, please follow-up with dao on IRC or other channels. We really want to get this failure fixed.
Comment 36 Maniac Vlad Florin (:vladmaniac) 2012-06-05 05:49:52 PDT
(In reply to Henrik Skupin (:whimboo) from comment #35)
> Vlad, please follow-up with dao on IRC or other channels. We really want to
> get this failure fixed.
Just sent Dao an e-mail, hopefully he's got some time to help
Comment 37 Dão Gottwald [:dao] 2012-06-05 06:06:30 PDT
As of Firefox 14, you can use this instead of DOMAttrModified: http://hacks.mozilla.org/2012/05/dom-mutationobserver-reacting-to-dom-changes-without-killing-browser-performance/
Comment 38 Maniac Vlad Florin (:vladmaniac) 2012-06-06 05:15:42 PDT
(In reply to Dão Gottwald [:dao] from comment #37)
> As of Firefox 14, you can use this instead of DOMAttrModified:
> http://hacks.mozilla.org/2012/05/dom-mutationobserver-reacting-to-dom-
> changes-without-killing-browser-performance/

Tried to instantiate nsIDOMMutationObserver in the mozmill test with no luck. I can't find 
the interface in Ci or any class in Cc 

Ran this JS snippet 

var x;
  for (x in Cc)
  dump("\n*** " + x + "\n");

to get all the domain names. 

Dao - any ideas how to use the MutationObserver constructor in Mozmill? I mean I want to create the observer object with 'new MutationOberver' and I think I need to create an instance of nsIDOMMutationObserver for that...thanks :)
Comment 39 Henrik Skupin (:whimboo) 2012-06-13 05:56:11 PDT
Vlad, any update here? It is failing again today:

http://mozmill-ci.blargon7.com/#/functional/report/e67171ea696231bb192f565615037a32
Comment 40 Maniac Vlad Florin (:vladmaniac) 2012-06-13 06:04:35 PDT
(In reply to Henrik Skupin (:whimboo) from comment #39)
> Vlad, any update here? It is failing again today:
> 
> http://mozmill-ci.blargon7.com/#/functional/report/
> e67171ea696231bb192f565615037a32

I cannot use DOMMutationObserver with Mozmill. Comment 38 explains the last thing I tried but without any luck. 
So the example in comment 37 is not applicable unless we find a way to override the MutationObserver contructor in Mozmill. 

Trying the example in comment 37 will make it clear what the problem is
Comment 41 Maniac Vlad Florin (:vladmaniac) 2012-06-13 06:09:10 PDT
Its failing also on aurora http://mozmill-ci.blargon7.com/#/functional/report/e67171ea696231bb192f56561504a360
Comment 42 Henrik Skupin (:whimboo) 2012-06-14 05:28:01 PDT
Not sure what the problem here is. It's kinda easy to get a MutationObserver instantiated. It's accessible as 'controller.window.MutationObserver' as explained in the referenced blog post in comment 37.
Comment 43 Maniac Vlad Florin (:vladmaniac) 2012-06-14 08:10:39 PDT
(In reply to Henrik Skupin (:whimboo) from comment #42)
> Not sure what the problem here is. It's kinda easy to get a MutationObserver
> instantiated. It's accessible as 'controller.window.MutationObserver' as
> explained in the referenced blog post in comment 37.

I get "MutationObserver is undefined" 
You get instantiate controller.window.nsIDOMMutationObserver successfully but that is the XPCOM interface

I'll try something else and come up with the results shortly
Comment 44 Maniac Vlad Florin (:vladmaniac) 2012-06-14 08:16:31 PDT
Also, it writing 

var something = controller.window.MutationObserver(..) I get 

{"message": "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIDOMMutationObserver.observe]", "name": "NS_ERROR_UNEXPECTED", "lineNumber": 87}}], "name": "testBackgroundTabScrolling.js::testScrollBackgroundTabIntoView", "filename": "/home/vladmaniac/Desktop/fixBackgroundScrolling/mozmill-tests/tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js", "failed": 1, "passed": 49}
Comment 45 Henrik Skupin (:whimboo) 2012-06-14 11:16:50 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #43)
> I get "MutationObserver is undefined" 
> You get instantiate controller.window.nsIDOMMutationObserver successfully
> but that is the XPCOM interface

Not sure what you are saying here. There is no nsIDOMMutationObserver interface property on the window object.

Works just fine for me:

  var observer = new controller.window.MutationObserver(function (mutations) {
      mutations.forEach(function (mutation) {
          console.log(mutation.type);
      });
  });
Comment 46 Maniac Vlad Florin (:vladmaniac) 2012-06-15 02:04:28 PDT
Created attachment 633438 [details] [diff] [review]
patch v2.0

Fixed. 

Perhaps it will be better to check also that the attribute is the right one which is added, but there is not other addition in the DOM for the scrollButtonDown Object. 

At least MutationObserver works now - needed a clean repo :( 
Let me know about further changes needed
Comment 47 Maniac Vlad Florin (:vladmaniac) 2012-06-15 02:07:47 PDT
Also, its probably better to add the MutationObserver in a shared module method, there are a few added code lines which complicates the test a bit
Comment 48 Henrik Skupin (:whimboo) 2012-06-15 02:21:04 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #46)
> At least MutationObserver works now - needed a clean repo :( 

Not sure why you needed a clean repo here. Nothing should have changed. Can you elaborate why?

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #47)
> Also, its probably better to add the MutationObserver in a shared module
> method, there are a few added code lines which complicates the test a bit

Feel free to add a bug for it. We could even get this added to Mozmill proper.
Comment 49 Henrik Skupin (:whimboo) 2012-06-15 02:25:29 PDT
Comment on attachment 633438 [details] [diff] [review]
patch v2.0

>+  // Check that the right scroll button flashes
>+  var insertedAttribute = false;

Please name it like 'hasHighlighted' so the test is better understandable.

>+  var observer = new controller.window.MutationObserver(function (mutations) {
>+    mutations.forEach(function (mutation) {
>+      insertedAttribute = true;
>+    });    

As you said yourself we want to wait for the right attribute. So please add that necessary code.

>+  var config = { attributes: true, childList: true, characterData: true };

Just move this up right before the MutationObserver instantiation.

>   controller.waitFor(function () {
>     return !scrollButtonDown.getNode().hasAttribute('notifybgtab');
>   }, "Hightlight should be removed immediately");

We will also have to use the MutationObserver here to be consistent.
Comment 50 Maniac Vlad Florin (:vladmaniac) 2012-06-18 01:11:35 PDT
Created attachment 633969 [details] [diff] [review]
patch v3.0

Fixed
Comment 51 Maniac Vlad Florin (:vladmaniac) 2012-06-18 01:12:58 PDT
Please keep in mind that DOMMutationObservers are introduced since Firefox 14, so this would apply for the following branches: 

* default
* mozilla-aurora
* mozilla-beta 

and not for 

* mozilla-release at the moment being Firefox 13
Comment 52 Henrik Skupin (:whimboo) 2012-06-18 02:23:30 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #51)
> Please keep in mind that DOMMutationObservers are introduced since Firefox
> 14, so this would apply for the following branches: 
> 
> * default
> * mozilla-aurora
> * mozilla-beta 

It would be more helpful to set the appropriate status flags. I will do that now. Also we never had failures with Firefox 13 or 10 ESR. So marking those branches as unaffected.
Comment 53 Henrik Skupin (:whimboo) 2012-06-18 02:32:29 PDT
Comment on attachment 633969 [details] [diff] [review]
patch v3.0

>+  var hasHighlighted = false;
>+  var highlightRemoved = false;

Please make use of an object here like 'highlight = { set: false, unset: false }'

>+  var config = { attributes: true, childList: true, characterData: true };

Please check for the right config we have to use in our case. This is a copy&paste from the example above.

>+  var observeHighlight = new controller.window.MutationObserver(function (mutations) {
>+    mutations.forEach(function (mutation) {
>+      hasHighlighted = scrollButtonDown.getNode().hasAttribute('notifybgtab');
>+      highlightRemoved = !hasHighlighted;

This is not what you want to do due to the asynchronous handling of the code. Please see the W3 documentation how to work with MutationObervers correctly:

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers
Comment 54 Maniac Vlad Florin (:vladmaniac) 2012-06-18 05:30:32 PDT
(In reply to Henrik Skupin (:whimboo) from comment #53)
> Comment on attachment 633969 [details] [diff] [review]
> patch v3.0
> 
> >+  var hasHighlighted = false;
> >+  var highlightRemoved = false;
> 
> Please make use of an object here like 'highlight = { set: false, unset:
> false }'

Good point

> 
> >+  var config = { attributes: true, childList: true, characterData: true };
> 

Investigated and our case will need only attributes: true

> Please check for the right config we have to use in our case. This is a
> copy&paste from the example above.
> 
> >+  var observeHighlight = new controller.window.MutationObserver(function (mutations) {
> >+    mutations.forEach(function (mutation) {
> >+      hasHighlighted = scrollButtonDown.getNode().hasAttribute('notifybgtab');
> >+      highlightRemoved = !hasHighlighted;
> 
> This is not what you want to do due to the asynchronous handling of the
> code. Please see the W3 documentation how to work with MutationObervers
> correctly:
> 
> http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers

Still need to look into the documentation. Hopefully I'll fix it by eod
Comment 55 Maniac Vlad Florin (:vladmaniac) 2012-06-18 05:31:14 PDT
Flags were reset again, sorry - I can't explain why this is happening
Comment 56 Maniac Vlad Florin (:vladmaniac) 2012-06-19 01:20:01 PDT
> 
> This is not what you want to do due to the asynchronous handling of the
> code. Please see the W3 documentation how to work with MutationObervers
> correctly:
> 
> http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers

Are you suggesting here to record the old value of the attribute using attributeOldValue? In our case would be 'undefined' because the attribute does not exist at first, and after the event gets fired it is created
Comment 57 Henrik Skupin (:whimboo) 2012-06-19 01:25:15 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #56)
> Are you suggesting here to record the old value of the attribute using
> attributeOldValue? In our case would be 'undefined' because the attribute
> does not exist at first, and after the event gets fired it is created

You cannot expect that this attribute doesn't exist! We are running other tests before this one, so it can happen at any time. I'm fairly sure the attribute which has been changed comes with the mutation object.
Comment 58 Maniac Vlad Florin (:vladmaniac) 2012-06-19 04:48:44 PDT
(In reply to Henrik Skupin (:whimboo) from comment #57)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #56)
> > Are you suggesting here to record the old value of the attribute using
> > attributeOldValue? In our case would be 'undefined' because the attribute
> > does not exist at first, and after the event gets fired it is created
> 
> You cannot expect that this attribute doesn't exist! We are running other
> tests before this one, so it can happen at any time. I'm fairly sure the
> attribute which has been changed comes with the mutation object.

We can get the attribute name by 'mutation.attributeName' and we can assert for the highlight attribute. Now need to figure out how do we check that the highlight disappears
Comment 59 Henrik Skupin (:whimboo) 2012-06-19 07:24:45 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #58)
> We can get the attribute name by 'mutation.attributeName' and we can assert
> for the highlight attribute. Now need to figure out how do we check that the
> highlight disappears

In this case we probably also get the value via attributeValue? That one you will have to evaluate.
Comment 60 Maniac Vlad Florin (:vladmaniac) 2012-06-19 07:38:54 PDT
> In this case we probably also get the value via attributeValue? That one you
> will have to evaluate.

mutations.attributeValue is 'undefined' 
also, the usage is not documented here 

http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers
Comment 61 Maniac Vlad Florin (:vladmaniac) 2012-06-19 08:14:41 PDT
Created attachment 634426 [details] [diff] [review]
patch v4.0

This time we are getting the attribute name from 'mutation.attributeName' so we are sure its the right one. 

We test for insertion of the attribute and for extraction of it, by 
element.getNode().hasAttribute(mutation.attributeName) 

We have it right this time
Comment 62 Henrik Skupin (:whimboo) 2012-06-19 08:22:53 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #60)
> > In this case we probably also get the value via attributeValue? That one you
> > will have to evaluate.
> 
> mutations.attributeValue is 'undefined' 
> also, the usage is not documented here 

This was an example which you shouldn't take 1-1. I don't want to specify all the steps you will have to do to get this fixed, but want to show directions. So please check how to get the attribute value.

Also see:
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-attribute

"Attributes have a name and value."
Comment 63 Maniac Vlad Florin (:vladmaniac) 2012-06-19 08:27:23 PDT
> "Attributes have a name and value."

Just thought I could somehow get the value from the mutation object. my proposed patch is uploaded; lots of thanks for guidance in this one
Comment 64 Henrik Skupin (:whimboo) 2012-06-19 08:30:02 PDT
Comment on attachment 634426 [details] [diff] [review]
patch v4.0

>+  var observeHighlight = new controller.window.MutationObserver(function (mutations) {
>+    mutations.forEach(function (mutation) {
>+      highlight.set = mutation.attributeName;
>+      highlight.unset = scrollButtonDown.getNode().hasAttribute(mutation.attributeName);
>+    });    

So once again. This observer is called twice for highlight and normal state. So this will not work. Also you can only evaluate the values if the mutation you get here is for the wanted attribute. Right now you are reacting on any attribute change.

So before you continue in trying to put a patch together, can you please come up with some pseudo-code how you would solve it?
Comment 65 Maniac Vlad Florin (:vladmaniac) 2012-06-20 01:27:15 PDT
I am reacting on any attribute change and save the attribute name, then verify I have inserted the correct attribute. 

I cannot react on the specific attribute because the attribute does not exist all the time and would trow an exception. I need to observe the element's node for attribute changes and not the attribute for changes. that is not possible in this case. 

As well as for the observer being called twice, I agree we are saving useless data, so my idea is to create two observer instances, one for insertedHighlight and one for removedHighlight 

My pseudocode: http://themaniak.pastebin.mozilla.org/1668530
Comment 66 Henrik Skupin (:whimboo) 2012-06-20 04:38:02 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #65)
> I am reacting on any attribute change and save the attribute name, then
> verify I have inserted the correct attribute. 

The two steps way is all not necessary. Only evaluate the attribute if it's the bghighlight one as given by the mutation.

> I cannot react on the specific attribute because the attribute does not
> exist all the time and would trow an exception. I need to observe the
> element's node for attribute changes and not the attribute for changes. that
> is not possible in this case. 

When the attribute changes I'm fairly sure that we also get the value via the mutation object. If you are not sure about please contact the developer who has implemented this feature in Firefox. I do not think that it is save to check the node for the attribute value at the time the mutation observer notifies us. Checking the node's attribute can already be too late and the attribute value could have been changed or the attribute been removed.

> As well as for the observer being called twice, I agree we are saving
> useless data, so my idea is to create two observer instances, one for
> insertedHighlight and one for removedHighlight 

Please think about the event flow. This proposal doesn't work because we will not be able to catch the second mutation notification. It's registered too late. Please use a single mutation observer. And also make sure to release it afterward to not cause a memory leak.

> My pseudocode: http://themaniak.pastebin.mozilla.org/1668530

Again, using external pages it not that helpful because content will be lost over time. Always add comments or attach a document to the bug. In cases like those comments are very helpful because you can query for those on Bugzilla.
Comment 67 Maniac Vlad Florin (:vladmaniac) 2012-06-20 05:45:53 PDT
> 
> Please think about the event flow. This proposal doesn't work because we
> will not be able to catch the second mutation notification. It's registered
> too late. Please use a single mutation observer. And also make sure to
> release it afterward to not cause a memory leak.
> 
I agree, that's why I used a single mutation observer in the first place. 
The thing is that the attribute value does not change at all. 
At the overflow of the tabstrip, the attribute "notifybgtab" is inserted in the node with the value = true. After a few moments, its being removed completely, but the value does not change. its just removed. the MutationObserver is sensing the insertion and deletion of the attribute, we can easily verify this by dumping mutations.type in the console, it will echo "attribute" twice. I can test that the attribute is inserted by verifying the attributeName, but I'm not sure how to deal with the attribute removal.
Comment 68 Maniac Vlad Florin (:vladmaniac) 2012-06-25 05:39:22 PDT
Currently waiting for dao's feedback on this
Comment 69 Henrik Skupin (:whimboo) 2012-07-04 04:54:04 PDT
Created attachment 639053 [details] [diff] [review]
Patch v5

So we shouldn't wait any longer on this bug. Given the slow pace I have taken the stab and modified the test so that the short highlight should be covered safely now.

There is one issue with latest Nightly builds, which have to be checked. MutationObserver doesn't seem to work reliable when Firefox is in the background. Especially in such a case we miss the first transition.

Vlad, would you be so kind to check for a regression range? The latest Aurora build is still working fine.
Comment 70 Maniac Vlad Florin (:vladmaniac) 2012-07-04 04:59:57 PDT
> There is one issue with latest Nightly builds, which have to be checked.
> MutationObserver doesn't seem to work reliable when Firefox is in the
> background. 

Do we have a bug for this or its something you've noticed while testing your patch?
Comment 71 Henrik Skupin (:whimboo) 2012-07-04 05:03:24 PDT
There is no bug existent yet for that issue. Having a quick check for a regression range would help me to file a better bug report about that regression.
Comment 72 Maniac Vlad Florin (:vladmaniac) 2012-07-04 05:49:54 PDT
(In reply to Henrik Skupin (:whimboo) from comment #71)
> There is no bug existent yet for that issue. Having a quick check for a
> regression range would help me to file a better bug report about that
> regression.

I see. Can I have a testcase to reproduce what you are seeing? 

Right now I did: 

1. import your patch 
2. ran testTabbedBrowsing/testBackroundScrolling.js locally 
3. put Firefox in the background by triggering an OS event which is more prior in Ubuntu, like window switching

result: 
I got this error 
"message": "The all tabs popup should have been opened", "fileName": "resource://mozmill/modules/utils.js", "name": "TimeoutError", "lineNumber": 447}}],

which is not the one related to our mutation observing code
Comment 73 Henrik Skupin (:whimboo) 2012-07-04 06:04:20 PDT
The updated waitFor method should have been failed. Not sure what you mean by 'triggering an OS event'. Simply select the terminal again right after you have issued the command.
Comment 74 Maniac Vlad Florin (:vladmaniac) 2012-07-04 06:17:42 PDT
(In reply to Henrik Skupin (:whimboo) from comment #73)
> The updated waitFor method should have been failed. Not sure what you mean
> by 'triggering an OS event'. Simply select the terminal again right after
> you have issued the command.

Not failing for me, sorry. What failure message do you get?
Comment 75 Dave Hunt (:davehunt) 2012-07-04 06:58:05 PDT
Comment on attachment 639053 [details] [diff] [review]
Patch v5

Looks great!
Comment 76 Henrik Skupin (:whimboo) 2012-07-04 07:18:00 PDT
Created attachment 639088 [details] [diff] [review]
Patch v5.1

We can even make it simpler. There is no need to check both states. The important one is the transition which removes the highlight. It will contain the old value of the attribute which we need to determine if a flash happened. With that we can even detect if another unwanted transition happens afterward.

Also this will kill the above mentioned problem when the mutation for the highlight is not getting caught. I will investigate that on another bug.
Comment 77 Dave Hunt (:davehunt) 2012-07-04 09:00:10 PDT
Comment on attachment 639088 [details] [diff] [review]
Patch v5.1

Even better! :)
Comment 78 Henrik Skupin (:whimboo) 2012-07-04 09:58:55 PDT
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/0ea5dabb151d
Comment 79 Henrik Skupin (:whimboo) 2012-07-05 06:48:49 PDT
Pushed to other affected branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/3515d19ea480 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2a74d9ff67bf (beta)
Comment 80 Alex Lakatos[:AlexLakatos] 2012-10-19 07:54:17 PDT
This happened again today on ESR Vista.
http://mozmill-ci.blargon7.com/#/functional/report/2e7486a8c08b5541b9074117e08dac14

Should we reopen this bug or open a new one?
Comment 81 Henrik Skupin (:whimboo) 2012-10-19 08:00:24 PDT
Looks like we should also land it on the esr10 branch.
Comment 82 Henrik Skupin (:whimboo) 2012-10-19 08:03:44 PDT
Well, Alex please file a new bug. It could be something different. As we have seen some months ago esr10 was not affected.
Comment 83 Alex Lakatos[:AlexLakatos] 2012-10-19 09:07:34 PDT
(In reply to Henrik Skupin (:whimboo) from comment #82)
> Well, Alex please file a new bug. It could be something different. As we
> have seen some months ago esr10 was not affected.
Logged bug 803581
Comment 84 Henrik Skupin (:whimboo) 2012-10-23 20:49:20 PDT
Not sure why but the patch landed also on esr10 by accident. It broke the test completely because MutationObserver is not available there.

I backed out the appropriate changeset:
http://hg.mozilla.org/qa/mozmill-tests/rev/44217a27492e

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