[TextSelection] selected text is covered by selection bubble in landscape mode

RESOLVED FIXED in FxOS-S10 (30Oct)

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jeremychen, Assigned: jeremychen)

Tracking

(Blocks: 1 bug)

unspecified
FxOS-S10 (30Oct)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 8 obsolete attachments)

46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review | Splinter Review
69.18 KB, image/png
Details
45.12 KB, image/png
Details
556.16 KB, application/zip
Details
89.38 KB, application/zip
Details
62.41 KB, patch
jeremychen
: review+
jeremychen
: ui-review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1207934 +++

Steps to reproduce:
1. Open a rotatable app. (ex. browser app)
2. Type 10 'a's, a space, and 10 'b's on rocketbar. It's essential that those 'b's are long enough to make the input scroll.
3. Long press to select all a's and 'b's.
4. Rotate the device to switch to landscape mode.

Actual result:
Selection bubble covers the selected text.

Expected result:
Selection bubble is displayed in the right position, which is the bottom of the selected text. The selected text should not be covered.

Build ID               20150930150203
Gaia Revision          be2da974ba076e06e2ff96cd3ac911769d3067a3
Gaia Date              2015-09-30 17:26:49
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9169f652fe5e69c2d77ac31929934a5bc3342e6e
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150930.182440
Firmware Date          Wed Sep 30 18:24:53 EDT 2015
Bootloader             L1TC000118D0
I think I just reduced the reproduce steps as follows:

1. Open a rotatable app. (ex. browser app)
2. Rotate the device to switch to landscape mode.
3. Type few text on rocketbar.
4. Long press to select any text and wake the selection bubble.
Summary: [TextSelection] selection bubble is displayed with wrong position after changing the orientation from portrait mode to landscape mode → [TextSelection] selected text is covered by selection bubble in landscape mode
According to comment 0 and my observations, there are two separate problems here:
1. After change from portrait mode to landscape mode, the bubble may not always update it's position.
2. Even if the position of bubble is updated, the bubble would be updated to the wrong position.

Bug #1207934 should fix the first one, so I change the bug summary to focus on the second problem.
Assignee: nobody → jeremychen
Re comment 2:

Bug 1207934 cannot fix the bubble position after changing from portrait mode to landscape mode. It only addresses the caret missing issue. I'll remove the dependency. Thank.
No longer depends on: 1207934
Created attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master
Comment on attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master

In the original code, we calculate posTop with (selectDialogBottom + distanceFromBottom), then examine whether the dialog is overlapped by keyboard with (frameHeight - distanceFromBottom - selectOptionHeight). Since we already add distanceFromBottom in the first place, there's no need to subtract distanceFromBottom in the examination.

Two things are also fixed:
1. Update distance parameter in AppTextSelectionDialog since selection caret has been enlarged for a while.
2. Examine whether selection dialog is overlapped by keyboard in the very end. The original logic adds offset to posTop "after" overlapping examination. This may cause a potential bug if adding offset to posTop leads dialog to be overlapped by keyboard.

Hi Tim, could you help review this? Thanks.
Attachment #8669572 - Flags: review?(timdream)
Comment on attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master

Is this testable? We should list considerations in unit tests.
Attachment #8669572 - Flags: review?(timdream) → review+
(In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18); please ni? to queue) from comment #6)
> Comment on attachment 8669572 [details] [review]
> [gaia] chenpighead:1210341 > mozilla-b2g:master
> 
> Is this testable? We should list considerations in unit tests.

Thank you for your fast review. About tests, what about adding an integration test in Bug 1115244?
(In reply to Jeremy Chen [:jeremychen] from comment #7)
> (In reply to Tim Guan-tin Chien [:timdream] (OOO Oct 9-18); please ni? to
> queue) from comment #6)
> > 
> > Is this testable? We should list considerations in unit tests.
> 
> Thank you for your fast review. About tests, what about adding an
> integration test in Bug 1115244?

On my second thoughts, I think adding unit tests in this bug (as Tim said) is right. I'm working on it. The integration test can be left in Bug 1115244 for testing the functionality of selection dialog.
Created attachment 8670111 [details]
screenshot on Aries - selecting text on rocketbar under landscape mode

After applying the wip patch, this bug has been solved on Aries.
Created attachment 8670112 [details]
screenshot on Flame - selecting text on rocketbar under landscape mode

After applying the wip patch, this bug has not been solved on Flame.
It can be seen from [1] and [2], the space between carets and keyboard on flame is still not enough. According to spec [3], since there's not enough space above & underneath, we vertically center align the utility bubble with the selected text, which is exactly the root cause why utility bubble covers selected text on rocketbar. I think it would be better to NI UX for spec supports before moving any further.

Hi Harly, Carol, could you take a look at this? Thank you.


[1] Attached:  screenshot on Aries - selecting text on rocketbar under landscape mode
[2] Attached:  screenshot on Flame - selecting text on rocketbar under landscape mode
[3] Bug 921965 Attached: FxOS 2.2 UX Spec_Text Selection_v1.4.pdf
Flags: needinfo?(hhsu)
Flags: needinfo?(chuang)
FWIW, I try to update the keyboard height in bug 1203455 and I was told we should not change the height of four-row layouts.

Comment 13

3 years ago
NI Tori to decide on the new UX for this issue.
Flags: needinfo?(hhsu) → needinfo?(tchen)
Hi Jeremy,
As our offline discussion, please help resize text selection caret and menu in order to figure out further image revise.
Thank you!
Flags: needinfo?(chuang)

Updated

3 years ago
Flags: needinfo?(tchen)
Created attachment 8673020 [details] [diff] [review]
wip - smaller dialog option [gaia patch]
Attachment #8673020 - Attachment is patch: true
Attachment #8673020 - Attachment mime type: text/x-patch → text/plain
Created attachment 8673426 [details] [diff] [review]
wip - reduce selection/touch caret size [gecko patch]
(In reply to Jeremy Chen [:jeremychen] from comment #15)
> Created attachment 8673020 [details] [diff] [review]
> wip - smaller dialog option [gaia patch]

After applying [1], quality of images in dialog options (select all/cut/copy/paste) seems been downgraded. Figuring out.

[1] attached: wip - smaller dialog option [gaia patch]
(In reply to Jeremy Chen [:jeremychen] from comment #17)
> After applying [1], quality of images in dialog options (select
> all/cut/copy/paste) seems been downgraded. Figuring out.
> 
> [1] attached: wip - smaller dialog option [gaia patch]

It's all about screen resolution (see [1]). Flashing gaia with proper GAIA_DPPX setting (1.5 for flame and 2.25 for aries) solves this problem.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Developing_Gaia/make_options_reference#High_resolution_image_assets
Created attachment 8673523 [details]
Caret_images_1014.zip

Hi Jeremy,
Could you help update the caret images?
I would like to see if the new size works in this landscape scenario, thanks!
Flags: needinfo?(jeremychen)
(In reply to Carol Huang [:Carol] from comment #19)
> Created attachment 8673523 [details]
> Caret_images_1014.zip
> 
> Hi Jeremy,
> Could you help update the caret images?
> I would like to see if the new size works in this landscape scenario, thanks!

Sure. Due to this change, I have to adjust few css attributes manually. Once I finish the work, I'll upload screenshots and send a ui-review request then.
Flags: needinfo?(jeremychen)
Created attachment 8674099 [details]
After_Applying_reduced_caret_images.zip

There are 2 folders, one for flame and one for aries. Screenshots before applying the patches are named as original-xxx.png, and those after applying the patches are named as reduced-xxx.png.
Attachment #8674099 - Flags: ui-review?(chuang)
Created attachment 8674102 [details] [diff] [review]
[gecko] (v1) Reduce accessiblecaret size. r=tlin
Attachment #8673020 - Attachment is obsolete: true
Attachment #8673426 - Attachment is obsolete: true
Comment on attachment 8674102 [details] [diff] [review]
[gecko] (v1) Reduce accessiblecaret size. r=tlin

Move ui-review request here.

Hi Ting-Yu, could you help review this? Thanks.
Attachment #8674102 - Flags: ui-review?(chuang)
Attachment #8674102 - Flags: review?(tlin)
Attachment #8674099 - Flags: ui-review?(chuang)
Comment on attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master

PR has been updated as follows:
1. Examine whether selection dialog is overlapped by keyboard in the very end.
2. Remove additional subtraction when examining if there is enough space underneath the selection rectangular.
3. Reduce selection dialog size (about ~0.9 times of the original size)
4. Add unit tests

Tim, some changes have been made to fulfill the new UX requirement. Could you review this again? Thank you.
Flags: needinfo?(timdream)
Comment on attachment 8674102 [details] [diff] [review]
[gecko] (v1) Reduce accessiblecaret size. r=tlin

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

browser/installer/package-manifest.in needs to be modified, too.

Extra nit: Since you use the new file name here, how about remove the "text_" prefix? Caret is not related to text really ...
Attachment #8674102 - Flags: review?(tlin) → feedback+

Comment 26

3 years ago
Created attachment 8674152 [details]
Screenshot 2015-10-15 17.23.55.png

After discussing offline with Carol, we want to adjust the layout for following two cases.

1. landscape/Flame (reduced-flame-landscape-tilt.png)
The utility bubble should be closer to the caret. Currently it closer to the keyboard than selected text which looks confusing.

2. landscape/Aries (original-aries-landscape-normal.png)
When there is enough space between carets, we want to put utility bubble underneath the text but between the carets. Please see the attachment.
Flags: needinfo?(jeremychen)
Created attachment 8674174 [details]
test_caret_size_w34h36.zip

After discussed with Carol offline, here is the reduced screenshot with caret size = 34 x 36. Attached have 2 more screenshots, flame_portrait_above.png and flame_portrait_underneath.png. They are attached to present that the distance from utility bubble are agree in both above and underneath cases.
Flags: needinfo?(jeremychen) → needinfo?(chuang)
Created attachment 8674645 [details]
(v2) test_caretsize_w34h36.zip

Add screenshots for aries. Pass ni here.
Attachment #8674174 - Attachment is obsolete: true
Flags: needinfo?(chuang)
(In reply to Jeremy Chen [:jeremychen] from comment #28)
> Created attachment 8674645 [details]
> (v2) test_caretsize_w34h36.zip
> 
> Add screenshots for aries. Pass ni here.

Oops, looks like I accidentally canceled the ni. Request again.
Flags: needinfo?(chuang)
Comment on attachment 8674102 [details] [diff] [review]
[gecko] (v1) Reduce accessiblecaret size. r=tlin

Hey Jeremy,
After replacing the new size of the images, the UI looks much better! thanks!
Flags: needinfo?(timdream)
Flags: needinfo?(chuang)
Attachment #8674102 - Flags: ui-review?(chuang) → ui-review+
Created attachment 8675521 [details]
Caret_images_1015_SMALLER.zip

Finalize caret size with 1x = w34 x h36. These files have been attached in gecko patch and got ui-review+ already. Upload them as a record.
Attachment #8673523 - Attachment is obsolete: true
Attachment #8674099 - Attachment is obsolete: true
Attachment #8674152 - Attachment is obsolete: true
Created attachment 8675522 [details] [diff] [review]
[gecko] (v2 carry ui-r+:chuang) Reduce accessiblecaret size. r=TYLin

1. Reduce caret size with 34 x 36
2. Address reviewer's comments
3. Fix tilt caret's hit test in marionette test

Ting-Yu, could you review this again? Thanks.
Attachment #8675522 - Flags: ui-review+
Attachment #8675522 - Flags: review?(tlin)
Attachment #8674102 - Attachment is obsolete: true
Comment on attachment 8675522 [details] [diff] [review]
[gecko] (v2 carry ui-r+:chuang) Reduce accessiblecaret size. r=TYLin

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

r=me with comment addressed. Thank you for the patch.

::: layout/base/tests/marionette/test_selectioncarets.py
@@ +308,5 @@
> +        elif self.carets_tested_pref == 'layout.accessiblecaret.enabled':
> +            caret_width = float(self.marionette.get_pref('layout.accessiblecaret.width'))
> +            caret_margin_left = float(self.marionette.get_pref('layout.accessiblecaret.margin-left'))
> +            tilt_right_margin_left = 0.41 * caret_width;
> +            tilt_left_margin_left = -0.39* caret_width;

Nit: a space before '*' please.

::: layout/style/ua.css
@@ +342,5 @@
>    display: none;
>  }
>  
>  div:-moz-native-anonymous.moz-accessiblecaret.normal > div.image {
> +  background-image: url("resource://gre/res/text_accessiblecaret.png");

You forgot to remove 'text_' prefix in this file.
Attachment #8675522 - Flags: review?(tlin) → review+
Hi Tim, I didn't notice that the ni request has been accidentally removed. =(
Set ni again for review request. Changes have been listed in Comment 24.
Flags: needinfo?(timdream)
Created attachment 8675540 [details] [diff] [review]
[gecko] (v3 carry r+:TYLin ui-r+:Carol) Reduce accessiblecaret size.

Address reviewer's comments.
Attachment #8675540 - Flags: ui-review+
Attachment #8675540 - Flags: review+
Attachment #8675522 - Attachment is obsolete: true
Carol and TYLin, thanks for your reviews. Let's wait for positive results from try. Then, we can land the gecko patch. \o/
Comment on attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master

Looks good!
Flags: needinfo?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #37)
> Comment on attachment 8669572 [details] [review]
> [gaia] chenpighead:1210341 > mozilla-b2g:master
> 
> Looks good!

Thanks for the review. =)
Gaia try looks good. Please check-in the gaia patch, and leave open for the gecko patch, thanks.
Keywords: checkin-needed, leave-open
Gecko try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25ab78e6562c except [OS X 10.6 XPCShell test], which is a known issue and not related to this bug. Please check-in gecko patch as well, thank you.

Updated

3 years ago
Target Milestone: --- → FxOS-S10 (30Oct)
Bug with repro steps in comment 1 has been fixed. However, the steps in comment 0 can still be reproduced in the latest pvt build. I've filed a followup bug for comment 0.
No longer blocks: 1216857
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.