Closed Bug 856595 Opened 12 years ago Closed 11 years ago

Intermittent test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: not zoomed out, got 1

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: RyanVM, Assigned: adw)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

patch
5.76 KB, patch
mikedeboer
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
https://tbpl.mozilla.org/php/getParsedLog.php?id=21302365&tree=Mozilla-Inbound Rev5 MacOSX Mountain Lion 10.8 mozilla-inbound opt test mochitest-1 on 2013-03-31 17:45:06 PDT for push 4222a898dbae slave: talos-mtnlion-r5-051 17:50:52 INFO - 101759 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Shouldn't zoom by vertical/positive pixel event when its lineOrPageDeltaY is 0: scrolled vertical 17:50:52 INFO - 101760 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Shouldn't zoom by vertical/positive pixel event when its lineOrPageDeltaY is 0: scrolled horizontal 17:50:52 INFO - 101761 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Shouldn't zoom by vertical/positive pixel event when its lineOrPageDeltaY is 0: zoomed 17:50:52 INFO - 101762 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: scrolled vertical 17:50:52 INFO - 101763 INFO TEST-PASS | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: scrolled horizontal 17:50:52 INFO - 101764 ERROR TEST-UNEXPECTED-FAIL | /tests/content/events/test/test_wheel_default_action.html | doTestZoom(aSettings=deltaX is reverted), Should zoom by vertical/positive pixel event when its lineOrPageDeltaY is 1: not zoomed out, got 1
Drew, I don't suppose this also falls into the "zoom bugs Drew's looking into" category, does it? :)
Flags: needinfo?(adw)
Sure.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
I think what's happening is: The test resets the zoom, which changes the visual zoom immediately and removes the page's zoom pref from content prefs. A little while later FullZoom is notified that a pref has been removed -- the same pref it just removed -- so it applies the default zoom to the page. But by that point the test has scrolled the mouse wheel, changing the zoom. So you end up with this sequence: test resets zoom, test scrolls mouse wheel, FullZoom sees the pref removal and effectively resets the zoom again. There was a similar intermittent failure a while ago, and the fix there was to stop FullZoom from updating the visual zoom when it saw an update for a pref whose visual zoom had already been applied. I think that may be the right behavior in all cases. https://tbpl.mozilla.org/?tree=Try&rev=7853b8e5a28e
(In reply to Drew Willcoxon :adw from comment #140) > I think what's happening is: The test resets the zoom, which changes the > visual zoom immediately and removes the page's zoom pref from content prefs. > A little while later FullZoom is notified that a pref has been removed -- > the same pref it just removed -- so it applies the default zoom to the page. > But by that point the test has scrolled the mouse wheel, changing the zoom. > So you end up with this sequence: test resets zoom, test scrolls mouse > wheel, FullZoom sees the pref removal and effectively resets the zoom again. > > There was a similar intermittent failure a while ago, and the fix there was > to stop FullZoom from updating the visual zoom when it saw an update for a > pref whose visual zoom had already been applied. I think that may be the > right behavior in all cases. > > https://tbpl.mozilla.org/?tree=Try&rev=7853b8e5a28e Any more luck with this? :-)
Bug 555120 fixes this one, too. Waiting on review.
(In reply to Drew Willcoxon :adw from comment #181) > Bug 555120 fixes this one, too. Waiting on review. Great! Thank you for the update :-)
Depends on: 555120
Target Milestone: --- → mozilla24
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch — — Splinter Review
I think the test just needs to wait for the actual zoom change after it synthesizes accel-0 to reset the zoom. This works locally, but I'll try it many times on try. https://tbpl.mozilla.org/?tree=Try&rev=034482b952d3
Comment on attachment 8366378 [details] [diff] [review] patch Try looks OK. Mike, you've worked on browser-fullZoom.js a little, so would you mind reviewing this change? The reason for using _executeSoon when firing the notification is that _getGlobalValue is sometimes async and sometimes sync. Using _executeSoon makes the notification behavior consistent from observers' points of view and allows them to always do this: resetZoom(); onZoomChange(...); // i.e., addObserver(function () {}, topic, false); Otherwise they'd have to add the observer first, which is more awkward IMO.
Attachment #8366378 - Flags: review?(mdeboer)
Blocks: fxdesktopbacklog
Whiteboard: [triage]
Comment on attachment 8366378 [details] [diff] [review] patch Review of attachment 8366378 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review atm because: (In reply to Drew Willcoxon :adw from comment #229) > The reason for using _executeSoon when firing the > notification is that _getGlobalValue is sometimes async and sometimes sync. It seems to me that ensuring _getGlobalValue to be _always_ async with an `_executeSoon()` would be a better improvement than handling this case only, right? No API consumer/ dev will expect that a continuation-passing-style function can be sync, so I think it'd be good to fix the core issue here.
Attachment #8366378 - Flags: review?(mdeboer)
Comment on attachment 8366378 [details] [diff] [review] patch The _getGlobalValue comment explains why it works like that. Please read it, but the point is that we want to keep paths sync whenever possible to have the best possible zooming UX. This behavior is by design, internal to FullZoom, and was already r+'ed when it was originally reviewed. So I'm requesting review again on the same patch.
Attachment #8366378 - Flags: review?(mdeboer)
Comment on attachment 8366378 [details] [diff] [review] patch Review of attachment 8366378 [details] [diff] [review]: ----------------------------------------------------------------- Hi Drew, thanks for explaining the story behind sync/async `_getGlobalValue`! With that in mind, I think this makes perfect sense. (I meant to post this yesterday, but somehow I forgot to push the button :/) ::: browser/base/content/browser-fullZoom.js @@ +329,5 @@ > this._getGlobalValue(browser.contentWindow, function (value) { > if (token.isCurrent) { > ZoomManager.setZoomForBrowser(browser, value === undefined ? 1 : value); > this._ignorePendingZoomAccesses(browser); > + this._executeSoon(function () { please add a comment explaining why this is here for posterity
Attachment #8366378 - Flags: review?(mdeboer) → review+
Thanks, landed with comment addressed. https://hg.mozilla.org/integration/fx-team/rev/d21d87af5436
https://hg.mozilla.org/mozilla-central/rev/d21d87af5436
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Drew, can you please request Aurora approval for this?
status-firefox27: --- → wontfix
status-firefox28: --- → affected
status-firefox29: --- → fixed
status-firefox-esr24: --- → wontfix
Flags: needinfo?(adw)
Target Milestone: mozilla24 → mozilla29
Comment on attachment 8366378 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): intermittent orange User impact if declined: none, test infrastructure will benefit though Testing completed (on m-c, etc.): 1 day on m-c Risk to taking this patch (and alternatives if risky): minor String or IDL/UUID changes made by this patch: n/a.
Attachment #8366378 - Flags: approval-mozilla-aurora?
Flags: needinfo?(adw)
Comment on attachment 8366378 [details] [diff] [review] patch Thanks Mike. Approved.
Attachment #8366378 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: fxdesktopbacklog
Whiteboard: [triage]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: