Closed Bug 589529 Opened 14 years ago Closed 14 years ago

Acer/Dell/Lenovo laptop trackpad scroll gesture doesn't work with 130078

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b5
Tracking Flags:
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: tnikkel, Assigned: khuey)

References

Details

(Keywords: regression)

Attachments

(1 file)

Patch
1.43 KB, patch
roc
: review+
Details | Diff | Splinter Review 
Using the scroll gesture on my Dell laptop trackpad changes the mouse icon to the scroll icon but doesn't actually scroll anything in a tryserver build with 130078 (http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/tnikkel@gmail.com-9a4eb8d17080/).

Feels like bug 507222 all over again.
Assuming 130078 and related bugs change our native widget structure, it's almost certainly the same.

Does anyone at Mozilla have contacts at Dell?
Bug 130078 removes the widgets from the content area. So on Windows we only have one widget per window.
not only Dell, maybe all trackpad/touchpad are included.
http://forums.mozillazine.org/viewtopic.php?p=9814997#p9814997
Yeah, this broke my Lenovo trackpoint as well.  Not sure if there's a workaround, but if not, this will be painful.
blocking2.0: --- → ?
So, my guess is that all of the shitty drivers are breaking because the top window is now MozillaUIWindowClass instead of MozillaWindowClass.  I'm going to try renaming them and see what happens.
Attached patch Patch — — Splinter Review 
This fixes it for me.  tn, want to take it for a spin?
Attachment #470205 - Flags: review?(roc)
Comment on attachment 470205 [details] [diff] [review]
Patch

This is OK. However, I think we need feedback from the accessibility folks.
Attachment #470205 - Flags: review?(roc) → review+
David, will this affect accessibility at all?
If this does affect a11y tools, it's very likely that they're already broken.
To recap a conversation I had on IRC, pre-130078 we had a widget structure that looked something like:

- MozillaUIWindowClass "Top level window"
--- MozillaContentWindowClass
----- MozillaWindowClass "Ima tab"
--- MozillaContentWindowClass
----- MozillaWindowClass "Imanother tab"

Post-130078 we have:

- MozillaUIWindowClass "Top level window"

This patch gives us:

- MozillaWindowClass "Top level window"

All of the structure is gone regardless, hence comment 9.
Marco, could you test how it affects on windows screen readers?
(In reply to comment #11)
> Try builds at
> ftp://ftp.mozilla.org/pub/firefox/tryserver-builds/khuey@kylehuey.com-21005e85314d/tryserver-win32/

Confirming this patch fixes the problem for me.
This should block beta5 IMO.
Confirming also that reverting to 082710 build or using the build from comment #11 both fix this regression for my Dell touchpad.  What are the barriers to checking this in?
Getting approval/blocking, which will happen tomorrow morning.
And verifying that it doesn't break accessibility tools.
(In reply to comment #19)
> And verifying that it doesn't break accessibility tools.

Not a tool actually but screen readers (though yes if firefox is a tool to browse the web then screen reader is a tool to use computer for blind people).

Iirc, previously screen readers used windows class name for their needs, I'm not sure whether they do this still. I hope Marco will check today a windows screen readers by try server build.

Btw, Robert, thank you for bringing an attention of a11y people to this bug.
Yeah, b5+ -- this will cause a flood of duplicate bugs and complaints amongst those on Lenovo or Dell laptops.
blocking2.0: ? → beta5+
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Assignee: khuey → nobody
Status: ASSIGNED → NEW
Assignee: nobody → khuey
Status: NEW → ASSIGNED
The try build in comment 11 fixes the problem for me too. Thanks Kyle!
I afraid we can't check whether a11y is affected by this patch until bug 591874 is fixed - a11y was broken on Windows this Friday.
(In reply to comment #20)

> Iirc, previously screen readers used windows class name for their needs, I'm
> not sure whether they do this still. I hope Marco will check today a windows
> screen readers by try server build.

Btw, screen readers use window class name for their work per discussion in bug 591874. This bug should be definitely checked if it doesn't break a11y before pushing.
With the try-server build from comment #11, screen readers are even more broken. There, I don't get any MSAA events/information except the title bar any more.
Thank you, Marco. I think whiteboard and keywords fields should be updated to prevent unwitting patch landing.
Removing checkin-request and whiteboard entry for now since this will break accessibility even further than it already is.
Keywords: checkin-needed
Whiteboard: [needs landing]
I see this patch removes MozillaUIWindowClass window class what I think negatively affects on screen readers. Jamie, could you give details how does NVDA rely on this?

Kyle, can you provide details how patch affects on structure of windows in Firefox?
(In reply to comment #28)
> I see this patch removes MozillaUIWindowClass window class what I think
> negatively affects on screen readers. Jamie, could you give details how does
> NVDA rely on this?
> 
> Kyle, can you provide details how patch affects on structure of windows in
> Firefox?

Comment 10 has a fairly good explanation.

I think we should ship beta5 with this patch for the reasons outlined in comment 21.  Fixing a11y should of course block a later release.
(In reply to comment #29)

> I think we should ship beta5 with this patch for the reasons outlined in
> comment 21.  Fixing a11y should of course block a later release.

then a11y issue should block beta5, otherwise we ship the unusable browser for blind people. On the another hand perhaps the unique fix of this a11y issue could be backing out the patch if there's no other way to fix this bug than changing the windows hierarchy or window class names.

Side note: all a11y fixes we're going to push after this patch is landed and before a11y fix is pushed are left untested because the browser is inaccessible. I'm not sure it's good idea.
Backing out 130078 isn't an option.  There's too much blocked on it.
(In reply to comment #31)
> Backing out 130078 isn't an option.  There's too much blocked on it.

As you pointed in bug bug 591874 an emulation can be done. This is great. And I guess similar things can be done for this bug as well. 

It's really huge problem for accessibility and it should be fixed asap. We need to get somebody working on this. And it's preferable to get fixed bug 591874 and then get correct fix for this bug so that a11y isn't broken. Otherwise I think further a11y development becomes a dangerous work because we don't have a way to test changes (and of course we loose an ability to get a testing from accessibility community as well).
FWIW, if we fix bug 591874 by emulating the previous widget structure that will fix this too.
(In reply to comment #33)
> FWIW, if we fix bug 591874 by emulating the previous widget structure that will
> fix this too.

Great. Ok, let's move a11y related discussion to bug 591874. Just please do not land this patch until that bug is fixed for the following reasons
1) further a11y development is not safe
2) no way to get feedback from a11y community
These are quite bad things for beta development.
The tradeoff here is between totally-busted a11y and somewhat-busted-scrolling on Windows laptops, as I understand it. Tricky, but I suspect that (sadly) it will benefit us more to take this fix and see how the Wild World of Mouse Drivers reacts to it and deal with the a11y regressions in a future beta. It is not a choice I make lightly, to be sure.

One possibility would be to check this patch in only on the b5-relbranch once we've tagged, which means that nightly a11y builds don't get totally hosed.
Having the same problem on my Acer Aspire 5532 laptop.  The problem began in the 2010/10/28 nightly.
(In reply to comment #36)
> Having the same problem on my Acer Aspire 5532 laptop.  The problem began in
> the 2010/10/28 nightly.

mmm, so you're from the future, eh? Please recheck your date. I think it's safe to say it affects all laptop touchpads.
Oops, that should have been 2010/08/28.
If 591874 isn't going to block beta5, then we should just land this patch.
Keywords: checkin-needed
Whiteboard: [needs landing]
(In reply to comment #28)
> I see this patch removes MozillaUIWindowClass window class what I think
> negatively affects on screen readers. Jamie, could you give details how does
> NVDA rely on this?
Whenever NVDA is dealing with Mozilla hwnds, it searches the hierarchy for a window which does not have a class name of "MozillaWindowClass" and uses that. This would previously have given us a "MozillaUIWindowClass" or "MozillaContentWindowClass" window. We do this because Gecko was previously inconsistent with the hwnds it returns; i.e. win events, WindowFromAccessibleObject() and Iaccessible::windowHandle return different window handles. Unfortunately, we never accounted for the possibility that MozillaWindowClass would be the highest Mozilla window in the hierarchy, so things break horribly with this patch.

We can probably handle this case on our side.

Btw, all of this craziness with window classes is documented here:
http://www.mozilla.org/access/windows/at-apis#content-window

Marco, it's possible that other screen readers aren't affected by this, even though they are affected by bug 591874. Have you tested with other screen readers?
Can we please land this patch? We'll want it in the tree for beta5, and if it's really interfering with nightly testing we can either take bug 591874 quickly or back it out until we have a fix for 591874
If this is in beta 5, can we at least have a note in the release notes stating that accessibility is very broken for most screen readers?
Summary: Dell laptop trackpad scroll gesture doesn't work with 130078 → Acer/Dell/Lenovo laptop trackpad scroll gesture doesn't work with 130078
I put the relnote tag on 591874.  Seems more logical to tag the bug that describes the brokenness, rather than the bug that did the breaking.
Keywords: relnote
It doesn't work on my Sony laptop either, I think the bug renaming should mention "laptop trackpad" and not name specific brands, at least imo.
http://hg.mozilla.org/mozilla-central/rev/3f499de2401d
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b5
(In reply to comment #10)
> To recap a conversation I had on IRC, pre-130078 we had a widget structure that
> looked something like:
> 
> - MozillaUIWindowClass "Top level window"
> --- MozillaContentWindowClass
> ----- MozillaWindowClass "Ima tab"
> --- MozillaContentWindowClass
> ----- MozillaWindowClass "Imanother tab"

This may look stupid, but is it possible for us to create three windows in a hierarchy like this:

- MozillaUIWindowClass "Top level window"
-- MozillaContentWindowClass
--- MozillaWindowClass "Rendering the entire chrome+content area"

The latter two windows will each be a child of the parent window and sized to the dimensions of the parent window as well.
I'm not sure but I think that might not be possible due to the changes we needed to make to fix bug 513162.
What changes do you mean exactly?
I'll let Jim explain it, he knows the details. But we used to have two windows at the top, one contained the view manager hierarchy which encompassed everything we draw. The other was the one that had the titlebar etc. We needed to merge those two into one so we could draw in the titlebar.
I'm guessing this seems to have caused Bug 507222 to re-appear again on my Thinkpad T61 running Windows XP.
Barry: yes, please wait for tomorrow's nightly in which it should be fixed.
Depends on: 593372
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: