If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
Widget: Win32
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: tnikkel, Assigned: khuey)

Tracking

({regression})

Trunk
mozilla2.0b5
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta5+)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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?
(Reporter)

Comment 2

7 years ago
Bug 130078 removes the widgets from the content area. So on Windows we only have one widget per window.

Comment 3

7 years ago
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.
Created attachment 470205 [details] [diff] [review]
Patch

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.
Try builds at ftp://ftp.mozilla.org/pub/firefox/tryserver-builds/khuey@kylehuey.com-21005e85314d/tryserver-win32/

Comment 12

7 years ago
Marco, could you test how it affects on windows screen readers?
Duplicate of this bug: 591772
(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.
Duplicate of this bug: 591763

Comment 17

7 years ago
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.

Comment 20

7 years ago
(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

Updated

7 years ago
Assignee: khuey → nobody
Status: ASSIGNED → NEW
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Keywords: regression
Keywords: checkin-needed
Whiteboard: [needs landing]
(Reporter)

Comment 22

7 years ago
The try build in comment 11 fixes the problem for me too. Thanks Kyle!

Comment 23

7 years ago
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.

Comment 24

7 years ago
(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.

Comment 26

7 years ago
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]

Comment 28

7 years ago
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.

Comment 30

7 years ago
(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.

Comment 32

7 years ago
(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.

Comment 34

7 years ago
(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.

Comment 36

7 years ago
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.

Comment 38

7 years ago
Oops, that should have been 2010/08/28.
Duplicate of this bug: 591613
If 591874 isn't going to block beta5, then we should just land this patch.
Keywords: checkin-needed
Whiteboard: [needs landing]

Comment 41

7 years ago
(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

Comment 43

7 years ago
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?

Updated

7 years ago
Summary: Dell laptop trackpad scroll gesture doesn't work with 130078 → Acer/Dell/Lenovo laptop trackpad scroll gesture doesn't work with 130078
Keywords: relnote
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.
Depends on: 591874
http://hg.mozilla.org/mozilla-central/rev/3f499de2401d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.
(Reporter)

Comment 48

7 years ago
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?
(Reporter)

Comment 50

7 years ago
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.

Comment 51

7 years ago
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.

Updated

7 years ago
Duplicate of this bug: 592173

Updated

7 years ago
Depends on: 593372
You need to log in before you can comment on or make changes to this bug.