Closed
Bug 1261671
Opened 9 years ago
Closed 9 years ago
[non-e10s] IME candidate window position is not located at correct position when Full zoom level <> 100%
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: alice0775, Assigned: jfkthame)
References
Details
(Keywords: inputmethod, regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
masayuki
:
review+
masayuki
:
feedback+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•9 years ago
|
||
[Tracking Requested - why for this release]: Bad UX
tracking-firefox46:
--- → ?
tracking-firefox47:
--- → ?
tracking-firefox48:
--- → ?
tracking-firefox-esr45:
--- → ?
Reporter | ||
Comment 2•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
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%
Reporter | ||
Comment 3•9 years ago
|
||
Screen capture : https://youtu.be/3rf_wEg14L8
Reporter | ||
Comment 4•9 years ago
|
||
The problem also happens on Ubuntu14.04 with fcitx-mozc.
OS: Windows 7 → All
Reporter | ||
Updated•9 years ago
|
Component: Widget: Win32 → Widget
Updated•9 years ago
|
Flags: needinfo?(masayuki)
Updated•9 years ago
|
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%
Reporter | ||
Updated•9 years ago
|
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%
Comment 6•9 years ago
|
||
(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)
Comment 7•9 years ago
|
||
s/during/between
Reproduce on about:newtab with e10s is enabled.
I think that "e10s" does not matter.
Updated•9 years ago
|
tracking-e10s:
--- → -
Comment 10•9 years ago
|
||
(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•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45327/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45327/
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
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?
Assignee | ||
Comment 14•9 years ago
|
||
This seems to work for me on OS X; try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1098d2369f5e.
Attachment #8739911 -
Flags: feedback?(masayuki)
Comment 15•9 years ago
|
||
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+
Assignee | ||
Comment 16•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → jfkthame
Comment 17•9 years ago
|
||
SO,will it landed ff46?
Updated•9 years ago
|
Attachment #8739643 -
Attachment is obsolete: true
Attachment #8739643 -
Flags: feedback?(jfkthame)
Comment 18•9 years ago
|
||
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+
Comment 19•9 years ago
|
||
(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.
Assignee | ||
Comment 20•9 years ago
|
||
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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 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?
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 23•9 years ago
|
||
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+
Assignee | ||
Comment 25•9 years ago
|
||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•