1261671 - [non-e10s] IME candidate window position is not located at correct position when Full zoom level <> 100%

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Alice0775 White]

Reproduced with MS-IME
Reproduced with disabled e10s.

Reproducible: always

Steps To Reproduce:
1. Disable e10s
2. Open about:home or any othe web page
3. Full Zoom
4. Attempt to input with IME

Actual Results:
Candidate window position is not located at correct position 


Expected Results:
Should be located at correct position

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Bad UX

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d977aeac761105ca8653ae6473f30672a6afbeb9&tochange=cd01c66f79b845822c6cbf73cc53f68bd292b700

Regressed by: cd01c66f79b8	Masayuki Nakano — Bug 1109410 Resolve CSS transform in ContentEventHandler::ConvertToRootViewRelativeOffset() r=roc

Comment 3

[Alice0775 White]

Screen capture : https://youtu.be/3rf_wEg14L8

Comment 4

[Alice0775 White]

The problem also happens on Ubuntu14.04 with fcitx-mozc.

Comment 5

[Jet Villegas (inactive)]

Why does it work in e10s?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jet Villegas (:jet) from comment #5)
> Why does it work in e10s?

Perhaps, the reason is, content process doesn't have chrome which is NOT zoomed. So, nsLayoutUtils::TransformFrameRectToAncestor() must not work fine if there are two or more zoom levels during the target frame and the reference frame.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

s/during/between

Comment 9

[baffclan]

Reproduce on about:newtab with e10s is enabled.
I think that "e10s" does not matter.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to baffclan from comment #9)
> Reproduce on about:newtab with e10s is enabled.
> I think that "e10s" does not matter.

about:newtab is not loaded in content process, you can check this with tooltip on tab's summary.

But anyway, this helps my test, thank you for the information!

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: MozReview Request: Bug 1261671 ContentEventHandler::ConvertToRootRelativeOffset() should resolve CSS transform per same FullZoom level documents WIP

Review commit: https://reviewboard.mozilla.org/r/45327/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45327/

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8739643 [details]
MozReview Request: Bug 1261671 ContentEventHandler::ConvertToRootRelativeOffset() should resolve CSS transform per same FullZoom level documents WIP

It seems that nsLayoutUtils::TransformFrameRectToAncestor() isn't aware of multiple PresContexts which have 2 or more full zoom levels. Therefore, I'm trying to fix this bug with using the method per zoom level. However, this still cause wrong position (although, the symptom is better).

jfkthame: Do you have any idea (or do you know a good person who is familiar with around this)?

Comment 13

[Jonathan Kew [:jfkthame]]

FWIW, I can reproduce the bug here on OS X, too, using the standard Japanese or Chinese IMEs.

I don't really have a good understanding of this code, but from experimenting a little, it seems to me that the problem is that when ContentEventHandler::ConvertToRootRelativeOffset converts the rect to be relative to the root frame, it returns this result in the root frame's appUnits; but the code using this method expects a result in units of the frame's own appUnits.

So can we fix this by simply making ConvertToRootRelativeOffset return its result in the original frame's appUnits rather than the root's appUnits? I've tried this in a few simple tests locally, and it seems to work fine with both CSS transforms and full-zoom. WDYT?

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different

This seems to work for me on OS X; try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1098d2369f5e.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8739911 [details] [diff] [review]
ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different

Excellent! Your idea really makes sense. Could you take this bug with this patch and request uplifting to Aurora due to a regression of UX?

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8739911 [details] [diff] [review]
ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different

OK, if you agree this makes sense (did you test it on Windows as well? I've only tried Mac OS X so far), let's get it landed in Nightly; and once it's confirmed as being fixed there, I'll request uplift.

Comment 17

[FateRover]

SO,will it landed ff46?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8739911 [details] [diff] [review]
ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different

Yeah, of course, I tested on Windows at +'ing feedback.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to FateRover from comment #17)
> SO,will it landed ff46?

No, I believe it's too late for Beta. Note that this regression hasn't been found for over 12 weeks. So, I guess that not so may user to use IME with zoom in/out.

Comment 20

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21ad62cfce1989cfb2881db01bacc236cc5917f6
Bug 1261671 - ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different. r=masayuki

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21ad62cfce19

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm sorry, it is too late for 46 as we are very close to release (April 26)
But we could still take this fix for 47. Can you request uplift to aurora?

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 8739911 [details] [diff] [review]
ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different

Approval Request Comment
[Feature/regressing bug #]: 1109410

[User impact if declined]: IME window misplaced if page is zoomed and e10s NOT active

[Describe test coverage new/current, TreeHerder]: tested manually (by me & Masayuki)

[Risks and why]: low, AFAIK this code is only used for IME support, and the patch is a no-op except when zoom is in effect

[String/UUID change made/needed]: none

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8739911 [details] [diff] [review]
ContentEventHandler::ConvertToRootRelativeOffset() should return the root-relative result in the frame's own appUnits, not the root's appUnits in the case when they're different

IME + Zoom related, seems low risk, Aurora47+

Comment 25

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27aca73b0ff7