Closed Bug 912941 Opened 11 years ago Closed 11 years ago

Fix scrollIntoView calls for earlier Firefox versions where it hasn't been implemented yet

Categories

(Testing Graveyard :: Mozmill, defect, P1)

Product:
Testing Graveyard
Component:
Mozmill
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: andrei, Assigned: andrei)

References

Details

(Keywords: regression, Whiteboard: [mozmill-2.0+][mozmill-1.5.23+][ateamtrack: p=mozmill q=2013q3 m=3])

Attachments

(3 files, 2 obsolete files)

0001-Bug-912941-Call-scrollIntoView-only-if-available.-r-.patch
1.49 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
followup_1.patch
1.44 KB, patch
whimboo
: review+
Details | Diff | Splinter Review
0001-Bug-912941-Update-code-for-controller.select-.-r-hsk.patch
911 bytes, patch
whimboo
: review+
Details | Diff | Splinter Review
I've only found 1 instance that is not properly handled: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozelement.js#L1033 This should be handled in the same way as the others are: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/mozelement.js#L366 This is the cause for bug 911101 Affects both mozmill 2.0 and 1.5
Attached patch patch 1 (master) (obsolete) — — Splinter Review
This fixes the problem on ESR17, other 2 instances of scrollIntoView are treated the same way.
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Attachment #800094 - Flags: review?(hskupin)
Attachment #800094 - Flags: review?(dave.hunt)
Attached patch patch 1 (hotfix-1.5) (obsolete) — — Splinter Review
Backport for hotfix-1.5
Attachment #800095 - Flags: review?(hskupin)
Attachment #800095 - Flags: review?(dave.hunt)
We also need a fix for Mozmill 1.5 so we can fix that regression. Andrei, please put the regressor as usual into the blocker field.
Keywords: regression
Whiteboard: [mozmill-2.0?] → [mozmill-2.0?][mozmill-1.5.23]
Comment on attachment 800094 [details] [diff] [review] patch 1 (master) Review of attachment 800094 [details] [diff] [review]: ----------------------------------------------------------------- This will not fix our problem if the element is really out of the viewport. With that patch we will no longer scroll at all to bring it into a visible area. Please check the code how it looked before.
Attachment #800094 - Flags: review?(hskupin)
Attachment #800094 - Flags: review?(dave.hunt)
Attachment #800094 - Flags: review-
Comment on attachment 800095 [details] [diff] [review] patch 1 (hotfix-1.5) Review of attachment 800095 [details] [diff] [review]: ----------------------------------------------------------------- Please do not upload backport patches until the one for master has been accepted. The some issues apply here.
Attachment #800095 - Flags: review?(hskupin)
Attachment #800095 - Flags: review?(dave.hunt)
Attachment #800095 - Flags: review-
(In reply to Henrik Skupin (:whimboo) from comment #5) > Comment on attachment 800094 [details] [diff] [review] > patch 1 (master) > > Review of attachment 800094 [details] [diff] [review]: > ----------------------------------------------------------------- > > This will not fix our problem if the element is really out of the viewport. > With that patch we will no longer scroll at all to bring it into a visible > area. Please check the code how it looked before. You are right, this won't fix that. But we are not failing anywhere with it... I have reinstalled the original code used for scrolling down (by pressing the down arrow key). ESR17 is very broken under Mozmill 2.0 (we have the test that originated this issue in bug 911101 passing now): http://mozmill-crowd.blargon7.com/#/functional/report/26d5854914a68aa13b333f28151ac85a
Attachment #800094 - Attachment is obsolete: true
Attachment #800095 - Attachment is obsolete: true
Attachment #802215 - Flags: review?(hskupin)
Comment on attachment 802215 [details] [diff] [review] 0001-Bug-912941-Call-scrollIntoView-only-if-available.-r-.patch Review of attachment 802215 [details] [diff] [review]: ----------------------------------------------------------------- Looks better. Now lets get the nits done and we can get it landed. Thanks! ::: mozmill/mozmill/extension/resource/driver/mozelement.js @@ +1040,5 @@ > + var selected = this.element.boxObject.QueryInterface(Ci.nsIMenuBoxObject).activeChild; > + if (item == selected) { > + break; > + } > + EventUtils.synthesizeKey("VK_DOWN", {}, ownerDoc.defaultView); I would feel better if we could simply take the code we had before for the else case. I don't want to introduce new regressions because we now operate on the active child. @@ +1047,1 @@ > item.click(); Nit: Lets add empty lines before and after the if condition.
Attachment #802215 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #8) > ::: mozmill/mozmill/extension/resource/driver/mozelement.js > @@ +1040,5 @@ > > + var selected = this.element.boxObject.QueryInterface(Ci.nsIMenuBoxObject).activeChild; > > + if (item == selected) { > > + break; > > + } > > + EventUtils.synthesizeKey("VK_DOWN", {}, ownerDoc.defaultView); > > I would feel better if we could simply take the code we had before for the > else case. I don't want to introduce new regressions because we now operate > on the active child. Well, that's what I did, this is the old code: https://github.com/mozilla/mozmill/commit/2684fb7418891cae0aff6c86b1297328d26259b0#L1L647 Should I change it to not use activeChild?
Flags: needinfo?(hskupin)
Oh, I was looking at the mozmill 1.5 code. There it will look differently. So for 2.0 it seems to be ok when we keep using activeChild. Please request review if all that applies.
Flags: needinfo?(hskupin)
Comment on attachment 802215 [details] [diff] [review] 0001-Bug-912941-Call-scrollIntoView-only-if-available.-r-.patch Review of attachment 802215 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you missed to request review here. I have checked the former code again - now with the hotfix-2.0 branch - and all looks fine. Thanks! ::: mozmill/mozmill/extension/resource/driver/mozelement.js @@ +1040,5 @@ > + var selected = this.element.boxObject.QueryInterface(Ci.nsIMenuBoxObject).activeChild; > + if (item == selected) { > + break; > + } > + EventUtils.synthesizeKey("VK_DOWN", {}, ownerDoc.defaultView); I would feel better if we could simply take the code we had before for the else case. I don't want to introduce new regressions because we now operate on the active child. @@ +1047,1 @@ > item.click(); Nit: Lets add empty lines before and after the if condition.
Attachment #802215 - Flags: review- → review+
Given that those are regressions, they have to block the next releases. Landed patches: https://github.com/mozilla/mozmill/commit/bbf5d5f2c18a69c595e9f2e41f46b890f9f3ee01 (master) https://github.com/mozilla/mozmill/commit/7a6c4c85bd7e44134448bb434b5f6c89ce90fb5f (hotfix-2.0) Andrei, please come up with a backport for Mozmill 1.5.
Flags: needinfo?(andrei.eftimie)
Whiteboard: [mozmill-2.0?][mozmill-1.5.23] → [mozmill-2.0+][mozmill-1.5.23+]
We will have to backout the previous landed changes. They fail under windows. The cause is a bit weird, I'll explain. We have this code: > this.element.click(); > if ("scrollIntoView" in item) { > item.scrollIntoView(); > } > else { > // Workaround for ESR17 where scrollIntoView is not available > // Scroll down until item is visible > for (var i = 0; i <= menuitems.length; ++i) { > var selected = this.element.boxObject.QueryInterface(Ci.nsIMenuBoxObject).activeChild; > if (item == selected) { > break; > } > EventUtils.synthesizeKey("VK_DOWN", {}, ownerDoc.defaultView); > } > } > item.click(); Originally this code wanted to do: 1) click on the dropdown/select 2) make sure we see the item we want to select 3) click on the item we want selected I remember working against this a longer time ago, and you could see the dropdown opening. Right now (OSX/Windows): 1) the first click does nothing (visually we do not see the dropdown open al all) 2) we are calling scrollIntoView to make sure the element is visible (yet the element is not drawn, so I'm not sure if that does anything at all) - in the mean time we've modified mozelement.mouseEvent() to call scrollIntoView() internally, so this step is actually obsolete since the method will be called when we issue the final click 3) we issue item.click() to select the desired item Well we can replace the whole code block above with just: > item.click() And we still select the correct item. This without even opening the dropdown/select. ***** Right now we fail on Windows (in the cookies from bug 911101 tests at least) because: - the dropdown/select is not opening - we click on down arrow - since the Preferences Pane is open, the down arrow event changes the active preferences pane Henrik, please backout the previously landed changes. I'm looking into why item.click(); works by itself, when this.element.click(); stopped opening the dropdown and how we should have this whole code block.
Flags: needinfo?(andrei.eftimie)
I'm not sure why we have to back this out. It's not getting worse with it in the code base. At least it works on Linux now. I would say you come up with a follow-up patch ASAP so we can get it fixed for real. Please make that your top priority item tomorrow. I want to get RC6 out, and this is blocking us from doing it. Thanks.
Priority: -- → P1
Attached patch followup_1.patch — — Splinter Review
I am still not sure _why_ we are able to click on an option directly. Fair enough `this.element.click()` does not open the select anyway, so it did work like this for some time. Still looking into _when_that started happening. Please look at this followup, all we do is click on the option we want selected, without _opening_ the dropdown first. (That's why I wanted us to back out the previous changes) The scrollIntoView() can safely be eliminated from here because it is already included in `item.click()` I'm uploading this patch now, because all our test are working great with it: OSX === ESR17 ----- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c402ed6 Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228c405ca5 Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1039ea48a9d69a5a1cc4fd228c407f58 l10n: http://mozmill-crowd.blargon7.com/#/l10n/report/1039ea48a9d69a5a1cc4fd228c408c8f Nightly ------- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c40e66e Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228c41171b Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1039ea48a9d69a5a1cc4fd228c420ed2 l10n: http://mozmill-crowd.blargon7.com/#/l10n/report/1039ea48a9d69a5a1cc4fd228c419d77 Windows ======= ESR17 ----- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c410562 Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228c412cef Nightly ------- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c423349 Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228c428ad3 Linux ===== ESR17 ----- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c443c5c Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228c435fb8
Attachment #807159 - Flags: feedback?(hskupin)
Attachment #807159 - Flags: feedback?(dave.hunt)
Comment on attachment 807159 [details] [diff] [review] followup_1.patch Review of attachment 807159 [details] [diff] [review]: ----------------------------------------------------------------- As discussed in the ask an expert meeting we want to go with this solution, but follow-up with another bug to investigate why that works. If another fix is necessary we can do that for e.g. 2.0.1.
Attachment #807159 - Flags: review+
Attachment #807159 - Flags: feedback?(hskupin)
Attachment #807159 - Flags: feedback?(dave.hunt)
Patch landed as: https://github.com/mozilla/mozmill/commit/3c5c3ff6e2539d4e22e3d714ddcb6295eaabba76 (master) https://github.com/mozilla/mozmill/commit/fd7aee6ed4bdd00f7db599c39e45c04936ae84af (hotfix-2.0) I will leave the bug open for the 1.5.23 fix.
Whiteboard: [mozmill-2.0+][mozmill-1.5.23+] → [mozmill-2.0+][mozmill-1.5.23+][ateamtrack: p=mozmill q=2013q3 m=3]
Andrei, please give us the patch for the 1.5 branch. I want to close this bug and get 1.5.23 released. Thanks.
Flags: needinfo?(andrei.eftimie)
Backported for hotfix-1.5 Same fix as for 2.0 Works very well. Ran a batch of testruns on OSX and Windows for ESR17 and Nightly: OSX === ESR17 ----- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf190ca http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf18fd8 Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cf26b88 http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cf2a756 l10n: http://mozmill-crowd.blargon7.com/#/l10n/report/1039ea48a9d69a5a1cc4fd228cf3c546 Addons: http://mozmill-crowd.blargon7.com/#/addons/report/1039ea48a9d69a5a1cc4fd228cf463ef Nightly ------- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf98bca http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf83171 Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cfa9608 http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cfa8f6a Windows ======= ESR17 ----- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf42d23 http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf2fb7c Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cf4bc4a http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cf4f181 Nightly ------- Functional: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf7e4c9 http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cf6e5de Remote: http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cface30 http://mozmill-crowd.blargon7.com/#/remote/report/1039ea48a9d69a5a1cc4fd228cfacdae
Attachment #808545 - Flags: review?(hskupin)
Attachment #808545 - Flags: review?(dave.hunt)
Flags: needinfo?(andrei.eftimie)
Comment on attachment 808545 [details] [diff] [review] 0001-Bug-912941-Update-code-for-controller.select-.-r-hsk.patch Review of attachment 808545 [details] [diff] [review]: ----------------------------------------------------------------- Andrei, have you proven that it works with the patch on bug 865640?
Thanks Andrei. I will take it as a go for us to get the final patch for Mozmill 1.5.23 landed.
Attachment #808545 - Flags: review?(hskupin)
Attachment #808545 - Flags: review?(dave.hunt)
Attachment #808545 - Flags: review+
Landed on hotfix-1.5: https://github.com/mozilla/mozmill/commit/4b9b03515692e91efe078a6c71b2947da3aec1e1 Also I'm still waiting for the follow-up bug you wanted to create.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(andrei.eftimie)
Resolution: --- → FIXED
Followup to investigate / update code is bug 920410
Flags: needinfo?(andrei.eftimie)
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: