11.36 KB, patch
|Details | Diff | Splinter Review|
47.50 KB, patch
|Details | Diff | Splinter Review|
7.23 KB, patch
|Details | Diff | Splinter Review|
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:126.96.36.199pre) Gecko/20090810 Shiretoko/3.5.3pre] (nightly) (W2Ksp4) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:188.8.131.52pre) Gecko/20090811 SeaMonkey/2.0b2pre] (nightly) (W2Ksp4) Work fine. [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090806 SeaMonkey/2.1a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/5c913c4662d8 + bug 444021 patch +http://hg.mozilla.org/comm-central/rev/4344cc9a28a7) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a2pre) Gecko/20090810 Minefield/3.6a2pre] (nightly) (W2Ksp4) 1. Load http://www.zonebourse.com/CAC-40-4941/analyse_technique-plein/ 1r. Between the bottom of the chart and the "Graphique statique / Graphique dynamique" links, there is a blank area. That area should contain a slider and some buttons.
Regressed between [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre] (nightly) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/f2a58ffcd00c) and [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre] (nightly) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/02f8bf10f441) Regression timeframe: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2a58ffcd00c&tochange=02f8bf10f441 I would suspect what roc landed... *** Workaround: Reload (or Back+Forward) fixes it. (But one has to know there is something missing in the first place :-/)
Assignee: nobody → roc
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
The problem here is that ScrollWindowEx with SW_SCROLLCHILDREN seems to not move descendants of child windows that don't intersect the scrolled rectangle, *even if* the immediate child window of the scrolled window *does* intersect the scrolled window. So this applet contains some child windows at the bottom, which are initially out of view. You scroll down, and the applet scrolls into view but its child windows stay where they are relative to the viewport, they effectively move down relative to the applet window (!). It's crazy...
For this bug we have to turn off Windows' SW_SCROLLCHILDREN in some cases where windowed plugins use child widgets. I discovered that we were sometimes turning it off because of the FAKESCROLLBAR widget we added to fool the Trackpoint driver. It's definitely unnecessary, and actually wrong, to create the FAKESCROLLBAR for plugin widgets, so let's stop doing that. However to do that we have to know at widget creation time that it's a plugin. eWindowType_plugin exists but hasn't been used. So this patch makes us use it, and changes most places we currently use eWindowType_child to also accept eWindowType_plugin. I'm also removing eWindowType_java because we don't use that (fortunately).
Attachment #402554 - Flags: review?(joshmoz)
To write a test for this bug we need the test plugin to have a child widget. We also need to be able to detect if the child widget has moved away from its correct position relative to its parent.
Attachment #402555 - Flags: review?(joshmoz)
The actual fix. This is rather grotesque but it seems like the best thing to do. I considered dropping all use of SW_SCROLLCHILDREN but it leads to noticeable visual weirdness when we scroll Flash. The test plugin's child widget is 10x10 positioned at the top-left corner of the plugin. So the test scrolls that child completely out of view, and then tries to scroll it back into view, at which point without the fix the test fails because SW_SCROLLCHILDREN doesn't move the plugin's child widget.
Attachment #402556 - Flags: review?(jmathies)
Kyle, just letting you know that in this bug I'm stopping us from creating the Trackpoint fake scrollbar for plugin widgets.
Whiteboard: [needs review]
(In reply to comment #5) > Created an attachment (id=402556) [details] > Part 3: fix > > The actual fix. This is rather grotesque but it seems like the best thing to > do. I considered dropping all use of SW_SCROLLCHILDREN but it leads to > noticeable visual weirdness when we scroll Flash. > > The test plugin's child widget is 10x10 positioned at the top-left corner of > the plugin. So the test scrolls that child completely out of view, and then > tries to scroll it back into view, at which point without the fix the test > fails because SW_SCROLLCHILDREN doesn't move the plugin's child widget. With SW_SCROLLCHILDREN removed, what happens with the child window that sits outside the parent? Was SW_SCROLLCHILDREN preventing these windows from being moved and now with it removed, windows handles moving these windows around correctly within the viewport?
I see so SW_SCROLLCHILDREN acts as a filtering mechanism on all child windows for our scrolling. Seems like the right fix would be to remove SW_SCROLLCHILDREN use completely and then address the flash weirdness. I suppose we need a fix for 1.9.2 that is low risk, so why don't we file a follow up bug to look into removing SW_SCROLLCHILDREN completely for the next release.
SW_SCROLLCHILDREN selects the subset of *descendant* (not just child) windows that intersect the given rectangle and moves them by the scroll amount. All other descendant windows keep the same absolute position. So a descendant window that doesn't move, but whose parent moves, effectively moves relative to its parent. That's bad, and this patch prevents it. When SW_SCROLLCHILDREN is not used, ScrollWindowEx does not change the positions of any windows. We have to manually move the windows ourselves. The "weirdness" I referred to is simply the effect you get if you call ScrollWindowEx to blit some area of your window around, and then move the plugin window manually using SetWindowPos or whatever. You see the window contents move and then just slightly later the plugin window moves. The effect is most visible if you drag the scroll thumb around, the plugin just seems to be lagging behind a little. I don't know of any way to prevent that other than to keep using SW_SCROLLCHILDREN when we can.
Turns out that nsChildView.mm doesn't set mWindowType from aInitData->mWindowType, so Part 1 broke plugins on Mac. This updated patch makes nsBaseWidget::BaseCreate set mWindowType to aInitData->mWindowType and mBorderStyles to aInitData->mBorderStyles, always, and removes all the other code that various platforms had to initialize those members.
Comment on attachment 402555 [details] [diff] [review] Part 2: Make test plugin have a child widget [Checkin: Comment 16] "Make Windows test plugin iin windowed mode" misspelled "in" +static void checkIs(int a, int b, const char* msg, string& error) Maybe call this "checkEquals" or something that communicates the operation.
Attachment #402555 - Flags: review?(joshmoz) → review+
Sorry, I attached a copy of Part 2 by mistake.
Comment on attachment 402556 [details] [diff] [review] Part 3: fix + test_plugin_scroll_consistency.html \ Didn't see this test in here anywhere, were you still planning on adding one?
Attachment #402556 - Flags: review?(jmathies) → review+
Sorry Jim, I forgot to hg add the test. Here's a patch which includes the test.
Whiteboard: [needs review] → [needs landing]
Spot the typo in part 1: + if (!mWindowType != eWindowType_plugin) return NS_ERROR_FAILURE; That caused some nice test failures.
Landed parts 2 and 3: http://hg.mozilla.org/mozilla-central/rev/f3c07913015f http://hg.mozilla.org/mozilla-central/rev/bfbd630be11a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 192 landing]
Er, those were parts 1 and 2. Part 3 was http://hg.mozilla.org/mozilla-central/rev/298772a42ecc
Whiteboard: [needs 192 landing] → [needs 192 landing: wait for both regression fixes first]
Attachment #402556 - Attachment is obsolete: true
Attachment #403091 - Attachment description: Part 1 v2 --- real patch → Part 1 v2 --- real patch [Checkin: Comment 16]
Attachment #402555 - Attachment description: Part 2: Make test plugin have a child widget → Part 2: Make test plugin have a child widget [Checkin: Comment 16]
Attachment #403147 - Attachment description: Part 3 v2 → Part 3 v2 [Checkin: Comment 17]
Target Milestone: --- → mozilla1.9.3a1
Priority: P1 → P2
No longer depends on: 520462
There were no regressions here. We don't need to hold this for an OS/2 build break fix.
Whiteboard: [needs 192 landing: wait for both regression fixes first] → [needs 192 landing]
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a1pre) Gecko/20091007 Minefield/3.7a1pre] (nightly) (W2Ksp4) V.Fixed
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.