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

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Jeremy Chen ([e-mail])]

+++ 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

Comment 1

[Jeremy Chen ([e-mail])]

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.

Comment 2

[Jeremy Chen ([e-mail])]

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.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] chenpighead:1210341 > mozilla-b2g:master

Comment 5

[Jeremy Chen ([e-mail])]

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.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master

Is this testable? We should list considerations in unit tests.

Comment 7

[Jeremy Chen ([e-mail])]

(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?

Comment 8

[Jeremy Chen ([e-mail])]

(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.

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: screenshot on Aries - selecting text on rocketbar under landscape mode

After applying the wip patch, this bug has been solved on Aries.

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: screenshot on Flame - selecting text on rocketbar under landscape mode

After applying the wip patch, this bug has not been solved on Flame.

Comment 11

[Jeremy Chen ([e-mail])]

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

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

[Harly Hsu[:harly]]

NI Tori to decide on the new UX for this issue.

Comment 14

[Carol Huang [:Carol]]

Hi Jeremy,
As our offline discussion, please help resize text selection caret and menu in order to figure out further image revise.
Thank you!

Comment 15

[Jeremy Chen ([e-mail])]

Attachment: wip - smaller dialog option [gaia patch]

Comment 16

[Jeremy Chen ([e-mail])]

Attachment: wip - reduce selection/touch caret size [gecko patch]

Comment 17

[Jeremy Chen ([e-mail])]

(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]

Comment 18

[Jeremy Chen ([e-mail])]

(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

Comment 19

[Carol Huang [:Carol]]

Attachment: 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!

Comment 20

[Jeremy Chen ([e-mail])]

(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.

Comment 21

[Jeremy Chen ([e-mail])]

Attachment: 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.

Comment 22

[Jeremy Chen ([e-mail])]

Attachment: [gecko] (v1) Reduce accessiblecaret size. r=tlin

Comment 23

[Jeremy Chen ([e-mail])]

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.

Comment 24

[Jeremy Chen ([e-mail])]

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.

Comment 25

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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 ...

Comment 26

[Tori Chen [:tori]]

Attachment: 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.

Comment 27

[Jeremy Chen ([e-mail])]

Attachment: 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.

Comment 28

[Jeremy Chen ([e-mail])]

Attachment: (v2) test_caretsize_w34h36.zip

Add screenshots for aries. Pass ni here.

Comment 29

[Jeremy Chen ([e-mail])]

(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.

Comment 30

[Carol Huang [:Carol]]

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!

Comment 31

[Jeremy Chen ([e-mail])]

Attachment: 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.

Comment 32

[Jeremy Chen ([e-mail])]

Attachment: [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.

Comment 33

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 34

[Jeremy Chen ([e-mail])]

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.

Comment 35

[Jeremy Chen ([e-mail])]

Attachment: [gecko] (v3 carry r+:TYLin ui-r+:Carol) Reduce accessiblecaret size.

Address reviewer's comments.

Comment 36

[Jeremy Chen ([e-mail])]

Carol and TYLin, thanks for your reviews. Let's wait for positive results from try. Then, we can land the gecko patch. \o/

Comment 37

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8669572 [details] [review]
[gaia] chenpighead:1210341 > mozilla-b2g:master

Looks good!

Comment 38

[Jeremy Chen ([e-mail])]

(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. =)

Comment 39

[Jeremy Chen ([e-mail])]

Gaia try looks good. Please check-in the gaia patch, and leave open for the gecko patch, thanks.

Comment 40

[Jeremy Chen ([e-mail])]

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.

Comment 41

[Carsten Book [:Tomcat]]

for gaia : https://github.com/mozilla-b2g/gaia/commit/a87245f8b25bdf22d1f3e154990b0439bef0db7e

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/56c6ed0b50e4

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/56c6ed0b50e4

Comment 44

[Jeremy Chen ([e-mail])]

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.