745077 - Touch event is not correct when the orientation changes

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

STR:

 1. rotate the device (sgs2)
 2. the homescreen is in landscape mode
 3. but the touch events are still in portrait coordinate.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

It looks like methods handling input coordinates need to consider current screen rotation and make adjustments accordingly. There is already a quick access to current rotation via nsScreenGonk, but width and height are problems. Querying screen manager seems not a good idea in such performance sensitive code.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Bug 745077 - Reconfigure InputReader on screen rotation.

Comment 3

[Michael Wu [:mwu]]

Regression from bug 737199

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 616871 [details] [diff] [review]
Bug 745077 - Reconfigure InputReader on screen rotation.

Review of attachment 616871 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with the requested changes.

::: widget/gonk/nsAppShell.cpp
@@ +704,5 @@
>      write(signalfds[1], "w", 1);
>  }
>  
> +void
> +nsAppShell::NotifyScreenChangesInternal()

Inline this into NotifyScreenChanges. Also rename NotifyScreenChanges to NotifyScreenRotation.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Michael Wu [:mwu] from comment #4)
> > +void
> > +nsAppShell::NotifyScreenChangesInternal()
> 
> Inline this into NotifyScreenChanges. Also rename NotifyScreenChanges to
> NotifyScreenRotation.

Hmm.. I need to access the private mReader. Do you want to expose gAppShell to nsWindow? 

NotifyScreenChanges does not only notify the orientation change but also the screen size change. But if you think NotifyScreenRotation fits here then I'm okay as well.

Comment 6

[Michael Wu [:mwu]]

(In reply to Kan-Ru Chen [:kanru] from comment #5)
> (In reply to Michael Wu [:mwu] from comment #4)
> > > +void
> > > +nsAppShell::NotifyScreenChangesInternal()
> > 
> > Inline this into NotifyScreenChanges. Also rename NotifyScreenChanges to
> > NotifyScreenRotation.
> 
> Hmm.. I need to access the private mReader. Do you want to expose gAppShell
> to nsWindow? 
> 

NotifyScreenChanges is part of nsAppShell, so it should be able to access mReader, no?

> NotifyScreenChanges does not only notify the orientation change but also the
> screen size change. But if you think NotifyScreenRotation fits here then I'm
> okay as well.

Yeah, it's really the only reason why the screen size would change, so I think notifyscreenrotation is fine.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Michael Wu [:mwu] from comment #6)
> > Hmm.. I need to access the private mReader. Do you want to expose gAppShell
> > to nsWindow? 
> > 
> 
> NotifyScreenChanges is part of nsAppShell, so it should be able to access
> mReader, no?

Oh, yes. So I will do gAppShell->mReader->... in it.
 
> > NotifyScreenChanges does not only notify the orientation change but also the
> > screen size change. But if you think NotifyScreenRotation fits here then I'm
> > okay as well.
> 
> Yeah, it's really the only reason why the screen size would change, so I
> think notifyscreenrotation is fine.

Got it.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Reconfigure InputReader on screen rotation. v1.1

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26dbb2e9d91d

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/26dbb2e9d91d