Video app to follow text selection pattern

RESOLVED FIXED in 2.2 S2 (19dec)

Status

Firefox OS
Gaia::Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: howie, Assigned: russn)

Tracking

unspecified
2.2 S2 (19dec)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Created attachment 8515835 [details]
FxOS 2.2 Text Selection Guidelines 03.pdf

* Input fields (input areas, input dialogs and search fields) are the only Building Blocks with selectable text. 

* HTML elements like buttons or links, or events like ontouchstart or onclick, should trigger the action when tapped rather than triggering text selection.
(Reporter)

Comment 1

4 years ago
The Gecko work is close to completion, the remaining part is to fix test case failures then pref it on in m-c bug 1092888. After the bug landed, text selection and cut/copy/paste can be triggered in apps on both editable and non-editable elements.

Gaia per app work in v2.2 is to follow UX guideline and leverage CSS -moz-user-select https://developer.mozilla.org/en-US/docs/Web/CSS/user-select to make certain elements non-selectable.

Before Gecko is pref on in m-c, Gaia can manually switch it on by the following two pref to test in advance:
pref("selectioncaret.enabled", on);
pref("selectioncaret.noneditable", on);

Comment 2

4 years ago
No impact since there aren't any input fields on video and based on https://bug1092972.bugzilla.mozilla.org/attachment.cgi?id=8515835
(Reporter)

Comment 3

4 years ago
This is a 2.2 feature.
feature-b2g: 2.2? → 2.2+

Updated

4 years ago
Assignee: nobody → rnicoletti
(Assignee)

Comment 4

4 years ago
The video app doesn't have any selectable fields. Does the video app needs to change wrt this bug?
Flags: needinfo?(hochang)
(Reporter)

Comment 5

4 years ago
Created attachment 8525894 [details]
Selection is triggered.png

Yes, we need to handle the case like the attachment, to make it not selectable. Currently the selection will be triggered by long press.
Flags: needinfo?(hochang)

Updated

4 years ago
QA Whiteboard: [2.2-feature-qa+]
(Assignee)

Comment 6

4 years ago
Created attachment 8531644 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26607
Attachment #8531644 - Flags: review?(wilsonpage)
Attachment #8531644 - Flags: review?(dflanagan)
Comment on attachment 8531644 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26607

Gaia-header is an external dependency that lives at http://github.com/gaia-components/gaia-header. If you need to make changes, you should submit pull-requests there and update 

Although you should be able to just define the property in your app without making a change to the gaia-header component.

gaia-header h1 { -moz-user-select: none }
Attachment #8531644 - Flags: review?(wilsonpage) → review-
(Assignee)

Comment 8

4 years ago
(In reply to Wilson Page [:wilsonpage] from comment #7)
> 
> Although you should be able to just define the property in your app without
> making a change to the gaia-header component.
> 
> gaia-header h1 { -moz-user-select: none }

I'm thinking we wouldn't want any gaia-header h1 element to be editable/selectable, regardless of the app that's using it, and therefore the change should be in the gaia-header component. Do you agree?
Flags: needinfo?(wilsonpage)
I'm happy to accept a PR on the gaia-header repo :)
Flags: needinfo?(wilsonpage)
(Assignee)

Updated

4 years ago
Depends on: 1107615
(Assignee)

Comment 10

4 years ago
Comment on attachment 8531644 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26607

I've updated this PR to remove the gaia-header changes. Those changes are part of separate bug and PR (bug 1107615)
Attachment #8531644 - Flags: review- → review?(wilsonpage)
Attachment #8531644 - Flags: review?(wilsonpage)
Attachment #8531644 - Flags: review?(dflanagan)
Attachment #8531644 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8531644 - Flags: review?(dflanagan)
Comment on attachment 8531644 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26607

r- because I'd like you to simplify this, or explain here what text you're making non-selectable and what text you're leaving selectable.

Page 12 of the attached pdf indicates that in the media apps nothing at all is selectable.  If that is true, then maybe it would be easiest to just put a single -moz-user-select:none on the body element and have it apply throughout the app.

Having said that, though, I think we should consult with UX about the text on the info screens and the text on the error overlays.  If they tell us that that text should be selectable, we can open a new bug and add -moz-user-select:text in the appropriate places.
Attachment #8531644 - Flags: review?(dflanagan) → review-
Omega,

The text selection guidelines attached to this bug only cover the main screens of the video app. What about text that appears in the "more info" screen?  (Play a video and then select the menu to see it).  And what about the text that appears in overlays like the ones you get if you launch the app with no videos on the phone or when the phone is plugged in for a USB Mass Storage transfer?

Updated

4 years ago
Flags: needinfo?(ofeng)
(Assignee)

Comment 13

4 years ago
(In reply to David Flanagan [:djf] from comment #11)
> Comment on attachment 8531644 [details] [review]
> Github PR: https://github.com/mozilla-b2g/gaia/pull/26607
> 
> r- because I'd like you to simplify this, or explain here what text you're
> making non-selectable and what text you're leaving selectable.
> 
> Page 12 of the attached pdf indicates that in the media apps nothing at all
> is selectable.  If that is true, then maybe it would be easiest to just put
> a single -moz-user-select:none on the body element and have it apply
> throughout the app.
> 
> Having said that, though, I think we should consult with UX about the text
> on the info screens and the text on the error overlays.  If they tell us
> that that text should be selectable, we can open a new bug and add
> -moz-user-select:text in the appropriate places.

I initially tried putting '-moz-user-select: none' on the 'body' element but the css rules aren't written to inherit from 'body'.

I'm sure the text in "more info" screen should also be non-selectable. I will make that change, but won't update the PR until we hear from Omega.

I'll also add the appropriate comment(s) explaining the strategy for making text non-selectable.
(Assignee)

Comment 14

4 years ago
(In reply to Russ Nicoletti [:russn] from comment #13)
> (In reply to David Flanagan [:djf] from comment #11)
> > Comment on attachment 8531644 [details] [review]
> > Github PR: https://github.com/mozilla-b2g/gaia/pull/26607
> > 
> > r- because I'd like you to simplify this, or explain here what text you're
> > making non-selectable and what text you're leaving selectable.
> > 
> > Page 12 of the attached pdf indicates that in the media apps nothing at all
> > is selectable.  If that is true, then maybe it would be easiest to just put
> > a single -moz-user-select:none on the body element and have it apply
> > throughout the app.
> > 
> > Having said that, though, I think we should consult with UX about the text
> > on the info screens and the text on the error overlays.  If they tell us
> > that that text should be selectable, we can open a new bug and add
> > -moz-user-select:text in the appropriate places.
> 
> I initially tried putting '-moz-user-select: none' on the 'body' element but
> the css rules aren't written to inherit from 'body'.
> 

What I meant by this is that I tried putting '-moz-user-select: none' on the 'body' element but I was still able to "select" the thumbnail details, etc. so it seems the thumbnails, etc. aren't inheriting the 'body' rules.
(Assignee)

Comment 15

4 years ago
On the assumption that text in the info view should not be selectable, I've updated the PR accordingly. I've also updated the initial PR such that it's more clear which elements are being explicitly made non-selectable. I'm waiting to set the review flag until we hear from Omega.
Sorry for late reply. All screens mentioned in comment 12 should be non-selectable.
Flags: needinfo?(ofeng)
(Assignee)

Updated

4 years ago
Attachment #8531644 - Flags: review- → review?(dflanagan)
Comment on attachment 8531644 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26607

It surprises me that -moz-user-select is not inherited by absolutely positioned elements.  I've updated MDN to explain that on this page: https://developer.mozilla.org/en-US/docs/Web/CSS/user-select

Since it does not inherit the way we expect, r+ to land the patch this way. Note, however, that for Gallery, Punam is doing "* { -moz-user-select: none }" instead of enumerating the individual elements that need it.  Either way is okay with me, but you might want to talk with her and decide on the same approach for both bugs.
Attachment #8531644 - Flags: review?(dflanagan) → review+

Updated

4 years ago
Target Milestone: --- → 2.2 S2 (19dec)
(Assignee)

Comment 18

4 years ago
Master: https://github.com/mozilla-b2g/gaia/commit/2aa46d15a2cf4b3726b336b8c7efd3ca358a468a

Regarding comment 17, the patch was changed to use Punam's gallery approach so that the video and gallery code is consistent.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.