Closed Bug 1261671 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Widget, defect)

Unspecified
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s - ---
firefox45 --- wontfix
firefox46 + wontfix
firefox47 + fixed
firefox48 + fixed
firefox-esr38 --- unaffected
firefox-esr45 - wontfix

People

(Reporter: alice0775, Assigned: jfkthame)

References

Details

(Keywords: inputmethod, regression)

Attachments

(1 file, 1 obsolete file)

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
Flags: needinfo?(masayuki)
[Tracking Requested - why for this release]: Bad UX
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
Blocks: 1109410
Summary: Candidate window position is not located at correct position when Fill zoom level <> 100% → IME candidate window position is not located at correct position when Fill zoom level <> 100%
Screen capture : https://youtu.be/3rf_wEg14L8
The problem also happens on Ubuntu14.04 with fcitx-mozc.
OS: Windows 7 → All
Component: Widget: Win32 → Widget
Summary: IME candidate window position is not located at correct position when Fill zoom level <> 100% → [non-e10s] IME candidate window position is not located at correct position when Fill zoom level <> 100%
Summary: [non-e10s] IME candidate window position is not located at correct position when Fill zoom level <> 100% → [non-e10s] IME candidate window position is not located at correct position when Full zoom level <> 100%
Why does it work in e10s?
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
Duplicate of this bug: 1261737
Reproduce on about:newtab with e10s is enabled.
I think that "e10s" does not matter.
(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 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)?
Attachment #8739643 - Flags: feedback?(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 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?
Attachment #8739911 - Flags: feedback?(masayuki) → feedback+
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.
Attachment #8739911 - Flags: review?(masayuki)
Assignee: nobody → jfkthame
SO,will it landed ff46?
Attachment #8739643 - Attachment is obsolete: true
Attachment #8739643 - Flags: feedback?(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

Yeah, of course, I tested on Windows at +'ing feedback.
Attachment #8739911 - Flags: review?(masayuki) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/21ad62cfce19
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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 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
Flags: needinfo?(jfkthame)
Attachment #8739911 - Flags: approval-mozilla-aurora?
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+
Attachment #8739911 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.