Closed Bug 1097398 Opened 5 years ago Closed 4 years ago

Match Android L text selection handles

Categories

(Firefox for Android :: Text Selection, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed
firefox49 --- verified
fennec 47+ ---

People

(Reporter: antlam, Assigned: TYLin)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(7 files, 10 obsolete files)

54.32 KB, image/png
Details
58 bytes, text/x-review-board-request
heycam
: review+
nalexander
: review+
Details
58 bytes, text/x-review-board-request
roc
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
Details
9.01 KB, application/zip
Details
1.88 KB, application/zip
Details
Like Bug 1097337, we should aim to match these orange handles too.
NI-ing Lucas here to put this on his radar for asset matching and what not.
Flags: needinfo?(lucasr.at.mozilla)
Depends on: 1056144
Flags: needinfo?(lucasr.at.mozilla)
Component: General → Text Selection
Is this just a resource change? Like handle shape/color etc? I'll need to ship new resources as part of bug 1215959 (experimental GeckoCarets -> AccessibleCarets)

Or does comment #1 imply you plan this bug as the vehicle for TextSelection via. FloatingToolbar implementation ?

ie: possible dup of Bug 1171110 ?
(In reply to Mark Capella [:capella] from comment #3)
> Is this just a resource change? Like handle shape/color etc? I'll need to
> ship new resources as part of bug 1215959 (experimental GeckoCarets ->
> AccessibleCarets)

I think so. Currently our handles are still styled like Android KitKat (?). I filed this to switch those handles to Lollipop ones

> Or does comment #1 imply you plan this bug as the vehicle for TextSelection
> via. FloatingToolbar implementation ?
> 
> ie: possible dup of Bug 1171110 ?

I think that's separate. That bug seems to be more about the floating action bar.
Hey Mark!

In the awesome text selection work you've been doing, I noticed the handles were different. I'd like to see if we can make them more like the ones here: 

http://www.google.com/design/spec/patterns/selection.html#selection-text-selection 

But, I'm not sure what "values" we are able to set. For now, I just want to use the same shapes as the doc ^ but make it more aligned with our color palette, what values do I need to provide?
Flags: needinfo?(markcapella)
I'm not sure what |values| you're interested in, but changing color seems easy enough / .png edit. The Caret resources are found here: [0]   ( accessiblecaretFOO.png )

There's (3) basic shapes (caret, tilt-left, tilt-right) and (4) sizes of each.

[0] http://mxr.mozilla.org/mozilla-central/source/editor/composer/res/
Flags: needinfo?(markcapella)
Antlam, if you're more a designer than a dev, I can change that for you pretty easily :-)

( Mine match my favorite color https://www.dropbox.com/s/13n0h0uetnm5qxm/bug1097398_greenHandles.png?dl=0 )

Is there a particular hex color code for the orange you need?
(In reply to Mark Capella [:capella] from comment #7)
> Antlam, if you're more a designer than a dev, I can change that for you
> pretty easily :-)
> 
> ( Mine match my favorite color
> https://www.dropbox.com/s/13n0h0uetnm5qxm/bug1097398_greenHandles.png?dl=0 )
> 
> Is there a particular hex color code for the orange you need?

Awesome, I'm just looking to use the same "image" for the carets as the doc in comment 5. That is, removing the current Nightly handles (that also do this weird rotating thing).

As for colors, Can we set them to our Fennec orange in our palette?

Let's go ahead and try that with a screenshot :) thanks Mark!
Flags: needinfo?(markcapella)
Attached patch bug1097398_chgCarets.diff (obsolete) — Splinter Review
This first bit is to create a mobile caret version of the accessible carets resources with unique color, shape, size, etc.

I thought there'd be a way to override the images during build via chrome, but the .css is using a resource:// vs. a chrome:// reference... still looking, so this bit might change.
Flags: needinfo?(markcapella)
Attachment #8698443 - Flags: feedback?(snorp)
Attached patch bug1097398_chgTilt.diff (obsolete) — Splinter Review
If I understand antlam's comment #6 correctly, the idea is to avoid the dynamic caret tilting, and just always use a stable image, similar to [0]

I'm using the shape provided by the accessiblecaret implementation, with a color change. If the shape needs further updating to better match Android, I might need a UI / graphics person to provided the polished product, as my GIMP skills are extremely basic.

[0] http://material-design.storage.googleapis.com/publish/material_v_4/material_ext_publish/0B6Okdz75tqQscGx0c3BfYktVeWs/patterns_selection_text03.png
cc:ing tylin in case he's interested :)
Comment on attachment 8698443 [details] [diff] [review]
bug1097398_chgCarets.diff

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

The ifdefs in ua.css seem wrong somehow -- not very scalable to additional versions. I think a frontend person should review.
Attachment #8698443 - Flags: feedback?(snorp) → feedback?(margaret.leibovic)
Great! The other thought was to vary the classname via pref: [0], perhaps using |"moz-mobilecaret"| ...

The code changes there while similarly minimal, felt equally unsatisfying (?)


[0] http://mxr.mozilla.org/mozilla-central/source/layout/base/AccessibleCaret.cpp?rev=169d9adca23f&mark=215-215#204
Comment on attachment 8698443 [details] [diff] [review]
bug1097398_chgCarets.diff

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

::: layout/style/ua.css
@@ +361,2 @@
>    background-image: url("resource://gre/res/accessiblecaret.png");
> +%endif

I agree with snorp that this seems fragile... ideally we should be able to override these image, but I'm not sure how to do that off the top of my head.

Redirecting to Nick to see if he knows how to do this.
Attachment #8698443 - Flags: feedback?(margaret.leibovic) → feedback?(nalexander)
Comment on attachment 8698443 [details] [diff] [review]
bug1097398_chgCarets.diff

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

As written, I expect this to fail, since only mobile/android installs the res/mobile* files, but editor/composer expects them unconditionally.  I don't understand why editor/composer isn't using chrome:// resources for this, which are intended to be overridden, but I'm sure there's history here.

Am I correct that we want accessible* for non-Android and mobile* for Android without other conditions?  If so, I propose we:

0) revert what appears to be a non-change to jar.mn;
1) remove conditional patch to ua.css entirely;
2) make editor/composer/moz.build include the res files if !Android;
3) move editor/composer/res/mobile* to mobile/android/res;
4) rename mobile/android/res/mobile* to match the names in editor/composer/res;
5) make mobile/android/res/moz.build include the res files (unconditionally, since it's always !!Android).

That makes the packager do the switch at build time, swapping out the files.  Does that seem reasonable?  Again, I'm surprised that these aren't chrome:// URLs, so that the jar.mn file just defines the relevant resources, and they can be easily overridden by themes, etc.
Comment on attachment 8698443 [details] [diff] [review]
bug1097398_chgCarets.diff

Alternately, we might be able to use CSS variables (and set them differently depending on platform/product), like in https://bugzilla.mozilla.org/show_bug.cgi?id=1208759.
Attachment #8698443 - Flags: feedback?(nalexander)
Maybe this helps! This is the visual I'm looking for. 

This uses our fennec orange (#FF9500). For the handles, the opacity is set at 0.9. For the text highlight, the opacity is set at 0.6.
Flags: needinfo?(alam) → needinfo?(markcapella)
Attached file img_texthandles.zip (obsolete) —
try these!
Attached file img_texthandle_middle.zip (obsolete) —
Oops! forgot the middle ones.

Can we get a build to test this?
Push to try with the new Caret resources from antlam ... I've mapped his HDPI / XHDPI / XXHDPI to the FFOS foo@1.5x.png, foo@2x.png, and foo@2.25x.png files.

The foo.png (basic 1x / MDPI) version I've *omitted* in this patch version ... assuming we don't have builds demanding it, and we wish to save the resource space. ( If you find a device where the handles don't seem to appear, this will be why :-) )

Not the final code, but this is fairly representative.
Flags: needinfo?(markcapella)
Attached patch bug1097398_chgCarets.diff (obsolete) — Splinter Review
I really like this fix for allowing platform specific resources.
Attachment #8698443 - Attachment is obsolete: true
Attachment #8699089 - Flags: review?(nalexander)
So, waiting for feedback on the build, and assets provided.
Flags: needinfo?(alam)
(In reply to Mark Capella [:capella] from comment #24)
> So, waiting for feedback on the build, and assets provided.

Ok, cool! Can you maybe post a screenshot of the new assets in action (on a phone/emulator) so I can see? :)
Flags: needinfo?(alam) → needinfo?(markcapella)
I can for the test devices I have available. They both display the same XXHDPI caret assets, so I'm hoping you or someone else (QA?) can test the other resolutions.

GS3: https://www.dropbox.com/s/gra6v73obnae8if/bug_1097398_orangeCaret_GS3.mp4?dl=0
N7 : https://www.dropbox.com/s/jvee3thnnsoi2ks/bug_1097398_orangeCaret_N7.mp4?dl=0

I believe the work here is done if the assets don't require further artwork changes. The part to make the selection foreground color / orange take an opacity seems to lead to code changes in areas unrelated to the Carets UI stuff (nsTextFrame, nsLookAndFeel, etc). I'd like to open a seperate bug and handle that change uniquely.
Flags: needinfo?(markcapella)
Comment on attachment 8699089 [details] [diff] [review]
bug1097398_chgCarets.diff

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

I have questions, but nothing serious.  If it works for you, it works for me!

::: editor/composer/moz.build
@@ +26,5 @@
>  ]
>  
>  FINAL_LIBRARY = 'xul'
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':

Let's add a comment:

```
# Fennec has custom carets.
```

Also, you want these for b2g (but not Fennec), right?  (I think `MOZ_WIDGET_TOOLKIT == 'gonk'` for b2g.)

::: mobile/android/installer/package-manifest.in
@@ -554,5 @@
>  @BINPATH@/res/html/*
>  @BINPATH@/res/language.properties
>  @BINPATH@/res/entityTables/*
>  #ifdef NIGHTLY_BUILD
> -@BINPATH@/res/accessiblecaret.png

This is not feature flagged? It's just NIGHTLY_BUILD?  We should feature flag it so it rides easier.

Are you certain we don't need the fallback versions here?
Attachment #8699089 - Flags: review?(nalexander) → review+
(In reply to Mark Capella [:capella] from comment #26)
> I can for the test devices I have available. They both display the same
> XXHDPI caret assets, so I'm hoping you or someone else (QA?) can test the
> other resolutions.
> 
> GS3:
> https://www.dropbox.com/s/gra6v73obnae8if/bug_1097398_orangeCaret_GS3.
> mp4?dl=0
> N7 :
> https://www.dropbox.com/s/jvee3thnnsoi2ks/bug_1097398_orangeCaret_N7.mp4?dl=0
> 
> I believe the work here is done if the assets don't require further artwork
> changes. The part to make the selection foreground color / orange take an
> opacity seems to lead to code changes in areas unrelated to the Carets UI
> stuff (nsTextFrame, nsLookAndFeel, etc). I'd like to open a seperate bug and
> handle that change uniquely.

I see the start caret awkwardly flipping every so often in the videos. Can we fix that please?

As per the transparencies, while it may be a different part of the code - but to the user it's the same "caret". So I'd like to tackle that as well before calling this "done". 

Also perhaps a build would help me gauge better? The video quality and size makes it hard for me to tell for sure if it's polished from a UI/UX point of view.

Thanks Capella! We're so close!
Flags: needinfo?(markcapella)
(In reply to Nick Alexander :nalexander from comment #27)
> Also, you want these for b2g (but not Fennec), right?  (I think
> `MOZ_WIDGET_TOOLKIT == 'gonk'` for b2g.)

The original accessiblecaret images are needed on b2g in production and on firefox desktop for automated testing.
Comment on attachment 8698445 [details] [diff] [review]
bug1097398_chgTilt.diff

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

::: layout/base/AccessibleCaretManager.cpp
@@ +357,5 @@
>  void
>  AccessibleCaretManager::UpdateCaretsForTilt()
>  {
>    if (mFirstCaret->IsVisuallyVisible() && mSecondCaret->IsVisuallyVisible()) {
> +    // Overlapping carets are tilted. Mobile style always prefers tilted.

I guess Fennec always wants mFirstCaret to be Appearence::Left, and mSecondCaret to be Appearance::Right regardless their x positions. So the logic should be:

if (sMobileStyle) {
  mFirstCaret->SetAppearance(Appearance::Left);
  mSecondCaret->SetAppearance(Appearance::Right);
} else if (mFirstCaret->Intersects(*mSecondCaret) {
  ...
}

In this way, the carets won't flip like Anthony said in comment 28.
Comment on attachment 8699089 [details] [diff] [review]
bug1097398_chgCarets.diff

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

::: editor/composer/moz.build
@@ +26,5 @@
>  ]
>  
>  FINAL_LIBRARY = 'xul'
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] != 'android':

This confuses me ... iiuic, nothing changes here, except we bypass loading these during build of mobile/android, and continue to export them for all other builds. The resources we bypass loading here for mobile/android builds are instead exported in mobile/android/moz.build, by your own design, yes?

::: mobile/android/installer/package-manifest.in
@@ -554,5 @@
>  @BINPATH@/res/html/*
>  @BINPATH@/res/language.properties
>  @BINPATH@/res/entityTables/*
>  #ifdef NIGHTLY_BUILD
> -@BINPATH@/res/accessiblecaret.png

There's other bits (4-ish?) to the implementation of AccessibleCaret that is simply hidden behind NIGHTLY_BUILD ... I'd lean against adding scope here (vs. a separate bug), but I'm actually unfamiliar with "feature-flagging".
nalexander: (forgot) I think between antlam, and mcomella, I was convinced we don't need the fallbacks, being comfortable with a fast fix if we find otherwise in m-c. If we're not collectively worried about ~1760 bytes, I can of course duplicate the smallest asset group and hook it in place.
Flags: needinfo?(markcapella)
antlam: can you use the build's attached in comment #22?

Also, we now have the caret transparencies via the caret image assets you provided. The orange foreground color applied to selected text by Gecko will require more thought and iiuic, a different set of reviewers so is best filed on a seperate track.

The changes should all meet up in m-c in any case, so there's no need to block here.

And finally, thanks to Tylin for the quick tweak we need to address the Caret twitch you mention. We can add that easily into this scope.
(mentioned mcomella couple comments back, noticed he's not cc:ed in, and might like to be)
BTW, if you want to change the blur bar between the selection highlight to be orange or something, tweak [1] in ua.css

If you don't want the bar at all, turn it off in [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/style/ua.css#353
[2] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/AccessibleCaretManager.cpp#317
Attached patch bug1097398_chgTilt.diff (obsolete) — Splinter Review
And this is why we try to watch scope :-)

Tylin, the last change requested (mobile tilt style) is a little more complex, owing to BIDI considerations.

Can you review the code? My C++ isn't yet where I'd like it to be, and you might spot some improvements.
Assignee: nobody → markcapella
Attachment #8698445 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8700179 - Flags: review?(tlin)
Attached patch bug1097398_chgTilt.diff (obsolete) — Splinter Review
Sorry for the spam, I attached the wrong patch :-(
Attachment #8700179 - Attachment is obsolete: true
Attachment #8700179 - Flags: review?(tlin)
Attachment #8700191 - Flags: review?(tlin)
Well, after digging through Gecko (Selection / Layout / Style, etc) code for longer than I care to admit, I finally see that changing the selection background color to the fennec_orange with an opacity of .6 is trivially simple.

Here's an updated / current set of patchs pushed to try for build testing :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=134e2fb66ac9
Comment on attachment 8700191 [details] [diff] [review]
bug1097398_chgTilt.diff

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

Mark, let me summarize what you want to do here. Correct me if I'm wrong.

You want mFirstCaret to be Appearance::Left on LTR node and Appearance::Right on RTL node, and mSecondCaret to be Appearance::Right on LTR node and Appearance::Left on RTL node. So it's possible that when you make selection from a RTL node to LTR node. Both carets will have Appearance::Right. Is is what you want?

::: layout/base/AccessibleCaretManager.cpp
@@ +357,5 @@
>  void
>  AccessibleCaretManager::UpdateCaretsForTilt()
>  {
> +  // Mobile style always prefers tilted.
> +  if (sMobileStyle) {

I feel it's more appropriate to use WritingMode on frames to check LTR and RTL. Check if the following four lines works for you :)

    WritingMode mFirstCaretWM = FindFirstNodeWithFrame(false, nullptr)->GetWritingMode();
    WritingMode mSecondCaretWM = FindFirstNodeWithFrame(true, nullptr)->GetWritingMode();
    mFirstCaret->SetAppearance(mFirstCaretWM.IsBidiLTR() ? Appearance::Left : Appearance::Right);
    mSecondCaret->SetAppearance(mSecondCaretWM.IsBidiLTR() ? Appearance::Right: Appearance::Left);

@@ +364,5 @@
> +  }
> +
> +  // Overlapping carets are tilted.
> +  if (mFirstCaret->Intersects(*mSecondCaret)) {
> +    if (mFirstCaret->IsVisuallyVisible() && mSecondCaret->IsVisuallyVisible()) {

It's wrong to swap the two if statements.
Attachment #8700191 - Flags: review?(tlin)
Thanks TYlin! That's a much nicer solution!

The behavior you suspect is correct. In mixed BIDI modes, the carets can appear to face the same directions. This is how Fennec, Chrome and Opera currently behave. (see test page I use: [0])

re : |It's wrong to swap the two if statements.|
yah, ouch, that was a fat-finger, and I've fixed it.

[0] https://www.dropbox.com/s/6qt6yvmwdulvm63/test_bug1215959.html?dl=0
Looking for feedback on the build provided in comment #38, prior to requesting final code review from snorp
Flags: needinfo?(alam)
(In reply to Mark Capella [:capella] from comment #41)
> Looking for feedback on the build provided in comment #38, prior to
> requesting final code review from snorp

Could I get a screenshot on white and dark?
Flags: needinfo?(alam) → needinfo?(markcapella)
(In reply to Mark Capella [:capella] from comment #43)
> I can quick do something like light/dark via readerView ...
> 
> https://www.dropbox.com/s/zjjscgig762p95e/bug_1097398_light.png?dl=0
> https://www.dropbox.com/s/pc6anwycohzpw3v/bug_1097398_dark.png?dl=0

Hey Mark,

These handles look way too big from the screenshots and comparing them to what's on chrome on my N7. Just to double check, are these using the right assets? I.e. calling the right DPI asset?

If you look at comment 18, you'll notice that the handle is only supposed to reach the baseline of the next line down. Whereas in your screenshots, they're hitting the x-height of the line 2 lines down.

Also, Can we remove the blue lines on both ends of the text selection to match the mock in comment 18 as well?

Thanks!
Flags: needinfo?(markcapella)
I've removed the delimiting selection bars [0].

See comment #21 and comment #26 again for background regarding the "xxhdpi" assets you see in the screenshots, rendered on my GS3, and N7, both @ 320 dpi/2.0 with X factor 2.0.

I agree they seem large, and depend on your design skills here. I can substitute anything else you may provide.

The controlling code is based on [1] (the ‘@media’ rule), and our specific implementation is: [2].


[0] https://www.dropbox.com/s/7rrgx8jq4adputs/bug_1097387_noBars.png?dl=0
[1] http://www.w3schools.com/cssref/css3_pr_mediaquery.asp
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css?rev=82bb82a9b038&mark=378-378,392-392,406-406#377
Flags: needinfo?(markcapella)
(In reply to Mark Capella [:capella] from comment #45)
> I've removed the delimiting selection bars [0].

Nice!

> See comment #21 and comment #26 again for background regarding the "xxhdpi"
> assets you see in the screenshots, rendered on my GS3, and N7, both @ 320
> dpi/2.0 with X factor 2.0.
> 
> I agree they seem large, and depend on your design skills here. I can
> substitute anything else you may provide.
> 
> The controlling code is based on [1] (the ‘@media’ rule), and our specific
> implementation is: [2].

I've traced the assets from the design guidelines so they should be accurate. I'm not sure what's going on here but it might be on the implementation end :\ sorry I can't help you there.

FWIW, I think the N7 is supposed to take XHDPI assets so maybe that's the problem?

See https://design.google.com/devices/ for more info?

> [0] https://www.dropbox.com/s/7rrgx8jq4adputs/bug_1097387_noBars.png?dl=0
> [1] http://www.w3schools.com/cssref/css3_pr_mediaquery.asp
> [2]
> http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.
> css?rev=82bb82a9b038&mark=378-378,392-392,406-406#377
Flags: needinfo?(markcapella)
Interesting ... I did indeed mis-speak earlier ...

See the entire gallery of carets Fennec vs. FF/OS [0]. I've color-tagged the Fennec carets and using a handy test-page ( provided by some guy whose name sounds familiar ;-) ) I see we're correctly selecting for the 2.0dppx assets. [1].

Is the scaling an issue here? For example:

accessible_caret_tilt_left@2px.png for Fennec: is 44x44 with no border, while
accessible_caret_tilt_left@2px.png for FFOS  : is 68x71 with a significant empty border

[0] https://www.dropbox.com/s/gjyowwbto6snp43/bug_1097398_gallery.png?dl=0
[1] https://www.dropbox.com/s/djl7g6wgukd75xz/bug_1097398_caret_analysis.png?dl=0
Flags: needinfo?(markcapella) → needinfo?(alam)
(In reply to Mark Capella [:capella] from comment #47)
> Interesting ... I did indeed mis-speak earlier ...
> 
> See the entire gallery of carets Fennec vs. FF/OS [0]. I've color-tagged the
> Fennec carets and using a handy test-page ( provided by some guy whose name
> sounds familiar ;-) ) I see we're correctly selecting for the 2.0dppx
> assets. [1].
> 
> Is the scaling an issue here? For example:
> 
> accessible_caret_tilt_left@2px.png for Fennec: is 44x44 with no border, while
> accessible_caret_tilt_left@2px.png for FFOS  : is 68x71 with a significant
> empty border
> 
> [0] https://www.dropbox.com/s/gjyowwbto6snp43/bug_1097398_gallery.png?dl=0
> [1]
> https://www.dropbox.com/s/djl7g6wgukd75xz/bug_1097398_caret_analysis.png?dl=0

I can't really tell much from these unfortunately.. can I just get a build to test on my device? that would be easiest.
Flags: needinfo?(alam) → needinfo?(markcapella)
Status:

So the last issue we have is the Fennec resource sets provided for the carets differing from those provided by FFOS.

Example for a 2.0dppx device we see:
   https://www.dropbox.com/s/a0mplt3czo0sfil/bug_1097398_sideBySide.png?dl=0

Fennec's lack the surrounding transparent borders, and when scaled, appear visually larger than the FF/OS assets, which isn't correct for us.

So we can either a) re-design our assets, building in the empty space, or b) override the .css

As an aside, I wonder if TYlin has any background on the FFOS design thoughts? Why they included the space?
Flags: needinfo?(markcapella) → needinfo?(tlin)
Can we just adjust for our own use case in the code? I don't think we need to keep this consistent with FFOS.
The caret images on Firefox OS were designed by Carol from the UX team. I believe the empty space around the caret image is to make dragging the caret easier since the user doesn't need to press onto the caret shape precisely to hold it.

If you want to adjust the image width, height, and the the horizontal shift of the caret images, we have prefs for that. https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/modules/libpref/init/all.js#4891-4893
Flags: needinfo?(tlin)
Attached patch bug1097398_chgScale.diff (obsolete) — Splinter Review
This may do it. I did some math and picked a downscale (44/68 ~= 0.647) which we can tweak in code.

Compare earlier [0] to current N7 [1] display in test.

Let's get a TRY build and test run: [2], with build downloadable: [3] 


[0] https://www.dropbox.com/s/djl7g6wgukd75xz/bug_1097398_caret_analysis.png?dl=0
[1] https://www.dropbox.com/s/xct6i22y7s0b1v2/bug_1097398_new.png?dl=0

[2] https://treeherder.mozilla.org/#/jobs?repo=try&author=markcapella@twcny.rr.com
[3] http://archive.mozilla.org/pub/mobile/try-builds/markcapella@twcny.rr.com-b18c1c0d41fa16bd1850ff29020ff42602182a12/try-android-api-11/
Flags: needinfo?(alam)
(In reply to Mark Capella [:capella] from comment #52)
> Created attachment 8700876 [details] [diff] [review]
> bug1097398_chgScale.diff
> 
> This may do it. I did some math and picked a downscale (44/68 ~= 0.647)
> which we can tweak in code.

Can you explain how you arrived at this number? I would really like to avoid just picking a number to downscale by simply by what "looks" closest to the mock. It tends to create a lot of problems down the line when we find the need to update the assets or tweak the design.

If we can just use the correct assets that would be best.

> Compare earlier [0] to current N7 [1] display in test.
> 
> Let's get a TRY build and test run: [2], with build downloadable: [3] 

(In reply to Mark Capella [:capella] from comment #53)
> sorry antlam, 9scroll bug in that one), can you test with this download ?
> 
> http://archive.mozilla.org/pub/mobile/try-builds/markcapella@twcny.rr.com-
> 3d04c8979ffdda8cde9b81a23fddcf8255b63e4f/try-android-api-11/

Just tested this on my 5X, the carets are getting closer in size. But they're also not "connected" to the highlight as the mock in comment 18. Let's fix that too.

Speaking of that highlight. How tall is it in dp? Can we set that? I'm noticing it's kind of short in your build, compared to my mock and chrome's UI.

And finally, did we end up figuring out what was happening with the incorrect assets back in comment 47? I'm finding it hard to follow along with what the issues we're trying to solve are and why the assets aren't just working. Do you just need new assets from me? Or can we just use the ones I gave since they're the right size? (but adjust for no padding in the code)
Flags: needinfo?(alam) → needinfo?(markcapella)
Chatted with antlam on irc and explained I need to move along some other projects ... we both think this is fairly close, but needs improvements.
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(markcapella)
ping Margaret! not sure if you know who might have time to push this over the finish line? :)
Flags: needinfo?(margaret.leibovic)
Attached patch bug1097398_chgCarets.diff (obsolete) — Splinter Review
Final version
Attachment #8699089 - Attachment is obsolete: true
Attached patch bug1097398_chgTilt.diff (obsolete) — Splinter Review
Final version.
Attachment #8700191 - Attachment is obsolete: true
(In reply to Anthony Lam (:antlam) from comment #56)
> ping Margaret! not sure if you know who might have time to push this over
> the finish line? :)

What's left to do here?

Capella has put a lot of effort into this, can we just land what he has here and refine it in a follow-up? This bug has dragged on for too long.
Flags: needinfo?(margaret.leibovic) → needinfo?(alam)
(In reply to :Margaret Leibovic from comment #59)
> (In reply to Anthony Lam (:antlam) from comment #56)
> > ping Margaret! not sure if you know who might have time to push this over
> > the finish line? :)
> 
> What's left to do here?

I think comment 54 sums it up. We both agree it's close, but not finished yet.

I'll attempt to list out what's missing:
 1) Carets are not "connected" to the highlight as the mock in comment 18
 2) How tall is it in dp? In the mock it is 16 dp
 3) I wanted to understand how we're re-sizing the assets to make sure they're fine on most devices

> Capella has put a lot of effort into this, can we just land what he has here
> and refine it in a follow-up? This bug has dragged on for too long.

While I definitely agree that Capella has put some great work into this, I'd like to complete the revisions in comment 54 (to make it look like the mock in comment 18) before closing this out. 

I'm concerned that this seems more "broken" than simply unpolished. So, I'd like to wrap this up here.
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
snorp, can someone from your team help wrap this up?
Flags: needinfo?(margaret.leibovic) → needinfo?(snorp)
(In reply to :Margaret Leibovic from comment #61)
> snorp, can someone from your team help wrap this up?

If this is about the handle assets, isn't it more frontendy stuff? If there are other issues we/I can help, but I don't think I am the right person for the UI stuff.
Flags: needinfo?(snorp) → needinfo?(margaret.leibovic)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #62)
> (In reply to :Margaret Leibovic from comment #61)
> > snorp, can someone from your team help wrap this up?
> 
> If this is about the handle assets, isn't it more frontendy stuff? If there
> are other issues we/I can help, but I don't think I am the right person for
> the UI stuff.

These patches are mostly in layout C++ code... but maybe the tweaks that are needed only require changes to the CSS?

Maybe ahunt could look into bringing this across the finish line. This bug blocks shipping the gecko text selection carets.
Flags: needinfo?(margaret.leibovic) → needinfo?(ahunt)
If I'm reading this right, it sounds like the best way to move forward is to get corrected assets. Anthony can you coordinate that with Mark if he has time? Or Andrew? Do you know which changed need to be made?
Flags: needinfo?(alam)
I'm adding a preference to control the appearance of the two blue bars at the ends of the selection highlight in bug 1241008. Once it landed, one can simply set the pref to false to turn off the blue bars.
Depends on: 1241008
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #64)
> If I'm reading this right, it sounds like the best way to move forward is to
> get corrected assets. Anthony can you coordinate that with Mark if he has
> time? Or Andrew? Do you know which changed need to be made?

I don't think it's the assets. They seem correct to me but I was unable to figure out (with :capella) what was happening.

I will ping :ahunt to see if we can figure this out. But he was on PTO today so we can revisit this tomorrow or day after.
Flags: needinfo?(alam)
Blocks: 1243542
Assignee: nobody → ahunt
tracking-fennec: --- → ?
TYLin, would you be interested in picking up this up to help us out?
tracking-fennec: ? → 47+
Flags: needinfo?(tlin)
I can pick this up, but I do not have much time this week to figure out all the remaining issues due to the new year holiday. Keep the NI to remind myself.
Flags: needinfo?(tlin)
Flags: needinfo?(tlin)
Duplicate of this bug: 1225657
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) (Lunar New Year 2/6 - 2/14, slow response) from comment #68)
> I can pick this up, but I do not have much time this week to figure out all
> the remaining issues due to the new year holiday. Keep the NI to remind
> myself.

Awesome, thanks. We're hoping to have this change in Fx47, but luckily there are still 4 more weeks of development time on Nightly.
Assignee: ahunt → tlin
Flags: needinfo?(tlin)
Flags: needinfo?(ahunt)
I think Margaret might have removed your NI by accident :) let me know when you' have questions though!
Flags: needinfo?(tlin)
Flags: needinfo?(tlin)
Anthony, would you help test the build in [1], and see whether it looks good to you?

I set the width and height of the bounding box of the caret to both 22px on 1dppx devices as the guideline in comment #1. Both the normal caret and the tilt caret images will be placed at the center of the 22px bounding box. And now the carets should be connected to the highlight as the mock in comment 18.

Here's the mapping table between the assets and the devices dppx:
- 1dppx (MDPI) -> Use HDPI assets (since lack of MDPI assets, but it should be scaled properly)
- 1.5dppx (HDPI) -> Use HDPI assets
- 2dppx (XHDPI) -> Use XHDPI assets
- 2.25dppx and above (XXHDPI) -> Use XXHDPI assets

[1] http://archive.mozilla.org/pub/mobile/try-builds/tlin@mozilla.com-cf34da394b2fe151a4532904b922e8975f971937/try-android-api-15/fennec-47.0a1.en-US.android-arm.apk
Flags: needinfo?(alam)
Attachment #8719717 - Flags: review?(roc)
Attachment #8719718 - Flags: review?(roc)
Attachment #8719719 - Flags: review?(nalexander)
Attachment #8719720 - Flags: review?(nalexander)
Comment on attachment 8719719 [details]
MozReview Request: Bug 1097398 Part 3 - Use Android L style carets assets on Fennec

https://reviewboard.mozilla.org/r/35069/#review31691

::: mobile/android/themes/core/content.css:341
(Diff revision 1)
> +  bottom: -11%; /* space between the blinking cursor and the caret */

Is this percentage measure good practice?  I find this very unlikely to work across all devices.

::: mobile/android/themes/core/jar.mn:141
(Diff revision 1)
> +  skin/images/texthandle_R_XXHDPI.png            (images/texthandle_R_XXHDPI.png)

Android's convention appears to be `texthandle-{left,right}-xxhdpi.png`: separators are hyphens, all lower case.

::: mobile/android/themes/core/jar.mn:142
(Diff revision 1)
> +% override chrome://global/skin/accessiblecaret/normal_1x.png chrome://browser/skin/images/texthandle_HDPI.png

I don't understand why this will work, since https://dxr.mozilla.org/mozilla-central/source/layout/style/ua.css#428 clearly wants resource://gre/res/accessiblecaret_tilt_right@2.25x.png, which this won't override.  Explain why you are using `_2x` and `__2.5x` and not just `@2.25x`.
Attachment #8719719 - Flags: review?(nalexander)
Attachment #8719720 - Flags: review?(nalexander) → review+
Comment on attachment 8719720 [details]
MozReview Request: Bug 1097398 Part 4 - Change text selection highlight color to fennec orange

https://reviewboard.mozilla.org/r/35071/#review31695

Sure.  I'll let you and antlam hammer this out.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #76)
> Anthony, would you help test the build in [1], and see whether it looks good
> to you?

Trying this build now on my N5, but I still see the original blue gecko carets... I've tried clearing the app and reinstalling too but still getting this. Not sure why
 
> [1]
> http://archive.mozilla.org/pub/mobile/try-builds/tlin@mozilla.com-
> cf34da394b2fe151a4532904b922e8975f971937/try-android-api-15/fennec-47.0a1.en-
> US.android-arm.apk
Flags: needinfo?(alam) → needinfo?(tlin)
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

https://reviewboard.mozilla.org/r/35065/#review31745

Better find another reviewer for this asset stuff
Attachment #8719717 - Flags: review?(roc)
Re comment #79:

Anthony, I'm sorry. N5 has dppx 3 according to [1]. My patch might not correctly override the assets with dppx > 2 as Nick pointed out in comment 77. I'll fix it in next patchset.

[1] http://dpi.lv/
Flags: needinfo?(tlin)
https://reviewboard.mozilla.org/r/35069/#review31691

> Is this percentage measure good practice?  I find this very unlikely to work across all devices.

Both height and width of div:-moz-native-anonymous.moz-accessiblecaret is specified in px in mobile.js, so the percentage measure of 'bottom' of div.image should be calculate relative to its parent's height. It should work like percentage 'margin-left' in [1]. FWIW, I test on Sony Z3C device which has 2dppx screen.

I don't know any other way to do this though ... 

[1] https://dxr.mozilla.org/mozilla-central/rev/6ea654cad929c9bedd8a4161a182b6189fbeae6a/layout/style/ua.css#378

> Android's convention appears to be `texthandle-{left,right}-xxhdpi.png`: separators are hyphens, all lower case.

OK. I'll rename the files to match the convention.

> I don't understand why this will work, since https://dxr.mozilla.org/mozilla-central/source/layout/style/ua.css#428 clearly wants resource://gre/res/accessiblecaret_tilt_right@2.25x.png, which this won't override.  Explain why you are using `_2x` and `__2.5x` and not just `@2.25x`.

I register chrome urls for these images in part 1. The double underscore is my bad, and will be fixed in next patch.
Awesome! this looks great :TYLin! 

Some fixes:
 - Can we continue showing the carets when the user scrolls the page? - this is what Chrome does.
 - the text highlight is a little bit too tall, it overlaps with itself when I highlight multiple lines of type. Can we shorten it? what height are we using for this right now? I may be able to provide better values :)
 - I know I said the transparency of the handles should be at 0.9 before. But I think we might need 0.95 since its a little hard to see over the text highlights
 

Looks good otherwise. Thanks again!
Flags: needinfo?(alam) → needinfo?(tlin)
(In reply to Anthony Lam (:antlam) from comment #89)
> Awesome! this looks great :TYLin! 

Thanks for the feedback!

> Some fixes:
>  - Can we continue showing the carets when the user scrolls the page? - this
> is what Chrome does.

I would love continue showing the carets as well. However this is probably not easy to do. File bug 1249201 for more technical discussion.

>  - the text highlight is a little bit too tall, it overlaps with itself when
> I highlight multiple lines of type. Can we shorten it? what height are we
> using for this right now? I may be able to provide better values :)

I saw this on some pages, too. I don't know anything about how the selection highlight is rendered nor the height we are using. Please file a bug under core::selection for further polish. Perhaps :mats knows more :)

>  - I know I said the transparency of the handles should be at 0.9 before.
> But I think we might need 0.95 since its a little hard to see over the text
> highlights

Would you prove a new assets which use transparency 0.95 so that I can update my patch with the new ones? 

BTW, is is convenient for you to export the assets into SVG format? I'd love to explore the possibility of using SVG as the caret image as a follow-up bug so that we could use only one assets and look sharp on all resolutions, and might help to rotate 90 degrees if we decide to support vertical text.

Also, is the new caret assets easy to drag for you? Because the new assets is smaller and without any padding in the image, I'd expect user might end up get page panning with higher probability than the original blue caret. I'd love to hear your feedback. If this seems to be a issue, we should revisit this in a follow-up bug.

> 
> Looks good otherwise. Thanks again!

You're welcome.
Flags: needinfo?(tlin) → needinfo?(alam)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #90)
> (In reply to Anthony Lam (:antlam) from comment #89)
> > Some fixes:
> >  - Can we continue showing the carets when the user scrolls the page? - this
> > is what Chrome does.
> 
> I would love continue showing the carets as well. However this is probably
> not easy to do. File bug 1249201 for more technical discussion.

Thanks for filing that. 

This is probably my biggest nit here since scrolling is so common, the flashing carets coming in and out of the page isn't a great experience. It'd be great if we could fix that.

> >  - the text highlight is a little bit too tall, it overlaps with itself when
> > I highlight multiple lines of type. Can we shorten it? what height are we
> > using for this right now? I may be able to provide better values :)
> 
> I saw this on some pages, too. I don't know anything about how the selection
> highlight is rendered nor the height we are using. Please file a bug under
> core::selection for further polish. Perhaps :mats knows more :)

Filed bug 1249327!

> >  - I know I said the transparency of the handles should be at 0.9 before.
> > But I think we might need 0.95 since its a little hard to see over the text
> > highlights
> 
> Would you prove a new assets which use transparency 0.95 so that I can
> update my patch with the new ones? 

Yes, I'll do that right now.

> BTW, is is convenient for you to export the assets into SVG format? I'd love
> to explore the possibility of using SVG as the caret image as a follow-up
> bug so that we could use only one assets and look sharp on all resolutions,
> and might help to rotate 90 degrees if we decide to support vertical text.

Sure, I can attach an SVG too.

> Also, is the new caret assets easy to drag for you? Because the new assets
> is smaller and without any padding in the image, I'd expect user might end
> up get page panning with higher probability than the original blue caret.
> I'd love to hear your feedback. If this seems to be a issue, we should
> revisit this in a follow-up bug.

I found this alright. I don't think it needs more padding right now. 

You bring up a good point though. That's why I think bug 1249201 is important here. Panning/scrolling might be a common interaction after text has been selected, _whether they intended to or not._

In Chrome, the carets stick around and the experience feels very continuous. For us, the carets disappear abruptly and reappears just as abruptly too. This experience is unsettling so maybe fixing this will help too. It'd be great to get if its not too hard.
Flags: needinfo?(alam)
Attached file icon_texthandle_95.zip
NI-ing for provided assets and comment 91 :)
Attachment #8698701 - Attachment is obsolete: true
Attachment #8698707 - Attachment is obsolete: true
Flags: needinfo?(tlin)
Attachment #8700876 - Attachment is obsolete: true
Attachment #8700969 - Attachment is obsolete: true
Attachment #8700970 - Attachment is obsolete: true
Looks great!
Flags: needinfo?(alam)
Nick, Anthony had done the UI review. Would you help review my patch? I've addressed and replied your previous review comments in comment #83. Thanks.
Flags: needinfo?(nalexander)
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

https://reviewboard.mozilla.org/r/35065/#review32487

Technically this looks fine, but see notes below.

::: layout/style/jar.mn:25
(Diff revision 3)
> +% skin global classic/1.0 %skin/classic/global/

I am very concerned that this pattern doesn't appear elsewhere in the platform (non application) code.  That suggests to me that this is *not* how the platform team does this type of application-specialization.  I will redirect this to another build peer who may have more context.

See, for example, https://dxr.mozilla.org/mozilla-central/search?q=%22skin%2Fclassic%22+-path%3Abrowser+-path%3Aobj-*+-path%3Atoolkit&redirect=false&case=false.

::: layout/style/jar.mn:26
(Diff revision 3)
> +   skin/classic/global/accessiblecaret/normal@1x.png        (images/accessiblecaret/normal@1x.png)

Why not put this in ``images/accessiblecaret``, so you don't have to include long paths all the time?
Attachment #8719717 - Flags: review?(nalexander)
Comment on attachment 8719719 [details]
MozReview Request: Bug 1097398 Part 3 - Use Android L style carets assets on Fennec

https://reviewboard.mozilla.org/r/35069/#review32489

Technically looks fine.

::: mobile/android/themes/core/jar.mn:133
(Diff revision 3)
> +  skin/images/texthandle-normal-hdpi.png         (images/texthandle-normal-hdpi.png)

I really don't understand your naming.  Why change ``tilt-left`` to ``texthandle-left``?  Why change "@1x" to "hdpi"?
Attachment #8719719 - Flags: review?(nalexander) → review+
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

Could you give some feedback on this approach?  I don't see any precedent for this approach in platform code and am concerned that there's an alternate implementation.
Flags: needinfo?(nalexander)
Attachment #8719717 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/35069/#review32489

> I really don't understand your naming.  Why change ``tilt-left`` to ``texthandle-left``?  Why change "@1x" to "hdpi"?

I preserve the naming style of the assets which Anthony attached instead of changing them to the same names as the platform default ones. I feel this might preseve more information on how the android assets are mapping to the platform ones.
https://reviewboard.mozilla.org/r/35065/#review32487

> I am very concerned that this pattern doesn't appear elsewhere in the platform (non application) code.  That suggests to me that this is *not* how the platform team does this type of application-specialization.  I will redirect this to another build peer who may have more context.
> 
> See, for example, https://dxr.mozilla.org/mozilla-central/search?q=%22skin%2Fclassic%22+-path%3Abrowser+-path%3Aobj-*+-path%3Atoolkit&redirect=false&case=false.

Sorry I do not know where is the proper place to put the assets for overriding. Is it better to put them in toolkit/themes/shared/jar.inc.mn?

> Why not put this in ``images/accessiblecaret``, so you don't have to include long paths all the time?

I mimic the path name as in
https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/jar.inc.mn
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #104)
> https://reviewboard.mozilla.org/r/35069/#review32489
> 
> > I really don't understand your naming.  Why change ``tilt-left`` to ``texthandle-left``?  Why change "@1x" to "hdpi"?
> 
> I preserve the naming style of the assets which Anthony attached instead of
> changing them to the same names as the platform default ones. I feel this
> might preseve more information on how the android assets are mapping to the
> platform ones.

Ah, I see.  These are new assets, so let's be consistent with Gecko for the name (use ``tilt-left``) and Android consistent for the density (use ``xxhdpi``).
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #105)
> https://reviewboard.mozilla.org/r/35065/#review32487
> 
> > I am very concerned that this pattern doesn't appear elsewhere in the platform (non application) code.  That suggests to me that this is *not* how the platform team does this type of application-specialization.  I will redirect this to another build peer who may have more context.
> > 
> > See, for example, https://dxr.mozilla.org/mozilla-central/search?q=%22skin%2Fclassic%22+-path%3Abrowser+-path%3Aobj-*+-path%3Atoolkit&redirect=false&case=false.
> 
> Sorry I do not know where is the proper place to put the assets for
> overriding. Is it better to put them in toolkit/themes/shared/jar.inc.mn?
> 
> > Why not put this in ``images/accessiblecaret``, so you don't have to include long paths all the time?
> 
> I mimic the path name as in
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/jar.inc.
> mn

I honestly don't know what the boundary here is.  If toolkit/ is the place this stuff lives, that's fine by me.  I'm going to ask Gijs, who has done many theming things like this before.

Gijs, TYLin is adding some text-selection-caret icons.  They need to be available to the Gecko platform.  Applications may override them for native look-and-feel.  There are few examples of this in pure platform code.  Does this type of stuff live in toolkit/?  Can we assume all platform consumers (b2g, and future b2g things?) have toolkit/, so this is the right division of responsibilities?  Feel free to redirect if appropriate.
Flags: needinfo?(gijskruitbosch+bugs)
toolkit/ is typically (but not only) XUL apps, so I hope / think b2g does not have access to it in general.

AIUI most of the styles used for platform code live in html.css / ua.css / forms.css and friends. Those are all in layout/style. I don't know what to do with resources used from within platform code, but I would assume the same thing applies there? I don't know if we do that for images, though.

Does that help? If not, I would suggest asking :dholbert if he knows (someone who knows).
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(nalexander)
(In reply to :Gijs Kruitbosch from comment #108)
> toolkit/ is typically (but not only) XUL apps, so I hope / think b2g does
> not have access to it in general.
> 
> AIUI most of the styles used for platform code live in html.css / ua.css /
> forms.css and friends. Those are all in layout/style. I don't know what to
> do with resources used from within platform code, but I would assume the
> same thing applies there? I don't know if we do that for images, though.
> 
> Does that help? If not, I would suggest asking :dholbert if he knows
> (someone who knows).

Oh, so the patch does move them to layout/style but then packages them into toolkit and references them with chrome://global/skin/ ? That does not seem right, especially because then third-party themes will have to provide them, and I don't think the build system will appreciate the files going into omni.ja (hopefully?) from layout/style...

It seems like android should override the styles specifically from CSS, and the location of the "default" files can remain as-is (ie editor).
(In reply to :Gijs Kruitbosch from comment #109)
> Oh, so the patch does move them to layout/style but then packages them into
> toolkit and references them with chrome://global/skin/ ? That does not seem
> right, especially because then third-party themes will have to provide them,
> and I don't think the build system will appreciate the files going into
> omni.ja (hopefully?) from layout/style...
> 
> It seems like android should override the styles specifically from CSS, and
> the location of the "default" files can remain as-is (ie editor).

If I understand this correctly, I don't need to register chrome url for the caret assets for overriding. I could add new android images in mobile/android/themes/core/jar.mn and overriding them by css background-images in mobile.css.

While I'm here, I do really want to move those platform caret images into layout/style/ since they're in editor only for historical reason, and they're used in layout/style/ua.css. Could I follow those arrow images in [1] to add those platform caret images? Or are they just bad examples either?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/jar.mn#16-21
Do you feel
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #110)
> (In reply to :Gijs Kruitbosch from comment #109)
> > Oh, so the patch does move them to layout/style but then packages them into
> > toolkit and references them with chrome://global/skin/ ? That does not seem
> > right, especially because then third-party themes will have to provide them,
> > and I don't think the build system will appreciate the files going into
> > omni.ja (hopefully?) from layout/style...
> > 
> > It seems like android should override the styles specifically from CSS, and
> > the location of the "default" files can remain as-is (ie editor).
> 
> If I understand this correctly, I don't need to register chrome url for the
> caret assets for overriding. I could add new android images in
> mobile/android/themes/core/jar.mn and overriding them by css
> background-images in mobile.css.

I believe that should work, yes.

> While I'm here, I do really want to move those platform caret images into
> layout/style/ since they're in editor only for historical reason, and
> they're used in layout/style/ua.css. Could I follow those arrow images in
> [1] to add those platform caret images? Or are they just bad examples either?
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/layout/style/jar.mn#16-21
> Do you feel

I'm not a layout peer, but without further context, adding more images analogous to the arrow.gif usage there seems fine. The "core" images just shouldn't be in toolkit/themes (or end up in global/skin).
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8719717 - Attachment description: MozReview Request: Bug 1097398 Part 1 - Make AccessibleCaret assets be able to override via chrome url → MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/
Attachment #8719717 - Flags: review?(mh+mozilla) → review?(dholbert)
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35065/diff/3-4/
Comment on attachment 8719718 [details]
MozReview Request: Bug 1097398 Part 2 - Add preferences to make carets always tilt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35067/diff/3-4/
Comment on attachment 8719719 [details]
MozReview Request: Bug 1097398 Part 3 - Use Android L style carets assets on Fennec

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35069/diff/3-4/
Comment on attachment 8719720 [details]
MozReview Request: Bug 1097398 Part 4 - Change text selection highlight color to fennec orange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35071/diff/3-4/
Comment on attachment 8719719 [details]
MozReview Request: Bug 1097398 Part 3 - Use Android L style carets assets on Fennec

Not sure who could help review part 1. Try :dholbert this time.

Nick, I follow the advice by Gijs to override platform caret assets in mobile/android/themes/core/content.css. Would you help review part 3 again? (Don't know how to change r+ to r? in mozreview ...)
Attachment #8719719 - Flags: review+ → review?(nalexander)
Attachment #8719717 - Flags: review?(dholbert)
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

https://reviewboard.mozilla.org/r/35065/#review33199

I do often review style-system code, but this should probably be reviewed by a style-system owner/peer [1] since it's moving a bunch of files into that directory.  (Also, I don't know much about packaging, so I feel unqualified to review on that front as well)

One concern that I'll voice before punting the review to someone else, though: are these images used at all in desktop builds?  I recall seeing them in mobile, but never on Desktop.  If they're not used in Desktop, it seems a bit wasteful that we'd now be including them in our desktop resources package. (They're 224 KB in size, which is small but not insignificant.)

[1] https://wiki.mozilla.org/Modules/Core#Style_System
(In reply to Daniel Holbert [:dholbert] from comment #117)
> One concern that I'll voice before punting the review to someone else,
> though: are these images used at all in desktop builds?  I recall seeing
> them in mobile, but never on Desktop.  If they're not used in Desktop, it
> seems a bit wasteful that we'd now be including them in our desktop
> resources package. (They're 224 KB in size, which is small but not
> insignificant.)
> 
> [1] https://wiki.mozilla.org/Modules/Core#Style_System

There's a pref for touch selection which you can toggle on desktop. AIUI kats is/was investigating doing that on Windows, but there are bugs that prevent turning it on by default. I don't know if that would actually mean these images would get used, though.

Also, based on the install manifest and them being in editor/ already, presumably they are already being packaged on desktop?
OK, thanks Gijs. (And I can confirm that e.g. "resource://gre/res/accessiblecaret_tilt_left.png" does show one of these images in my desktop Nightly build right now.)

So, I'll withdraw my concern in comment 117, given that the resources are already packaged for Desktop, and given they're intended to be used on Desktop (with touch selection).
BTW, the images are used in desktop build for marionette test and some reftest-style mochitest.
https://reviewboard.mozilla.org/r/35065/#review33199

Daniel, thank you for help and the question. I'll find a style system peer for the review.
Attachment #8719717 - Flags: review?(nalexander)
Attachment #8719717 - Flags: review?(cam)
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

https://reviewboard.mozilla.org/r/35065/#review33333

r=me on the layout/style/ changes.

::: layout/style/jar.mn:33
(Diff revision 4)
> +   res/accessiblecaret-tilt-right@2.25x.png  (res/accessiblecaret-tilt-right@2.25x.png)

(In the future we might want to move the other res/ resource files into the res/ directory, just to avoid cluttering up layout/style/.)

::: layout/style/ua.css:373
(Diff revision 4)
> -  background-image: url("resource://gre/res/accessiblecaret.png");
> +  background-image: url("resource://gre-resources/accessiblecaret-normal@1x.png");

I forget the distinction between resource://gre/res/ and resource://gre-resources/ but I assume this all works.
Attachment #8719717 - Flags: review?(cam) → review+
Comment on attachment 8719717 [details]
MozReview Request: Bug 1097398 Part 1 - Move AccessibleCaret default assets to layout/style/

https://reviewboard.mozilla.org/r/35065/#review33829

This is a packager skim and a stamp.  If a layout peer is happy, I'm fine with it all.

::: layout/style/jar.mn:21
(Diff revision 4)
>     res/arrowd-right.gif (arrowd-right.gif)

Further to heycam -- consider filing a mentor ticket for these other resources.  A good first bug, I think :)
Attachment #8719717 - Flags: review?(nalexander) → review+
Comment on attachment 8719719 [details]
MozReview Request: Bug 1097398 Part 3 - Use Android L style carets assets on Fennec

https://reviewboard.mozilla.org/r/35069/#review33833

If antlam's happy, I'm happy.

Sorry for the long and frequently delayed review process, TYLin.  This was a hard ticket for me to review -- right on the edge of my competency -- so thanks for your patience.
Attachment #8719719 - Flags: review?(nalexander) → review+
Removing NI -- review is looking good.  Thanks Gijs, dholbert, heycam, (others).
Flags: needinfo?(nalexander)
Status: NEW → ASSIGNED
> Further to heycam -- consider filing a mentor ticket for these other
> resources.  A good first bug, I think :)

I filed bug 1252368 for this :)
Depends on: 1252926
Blocks: 1252950
Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Nightly 49.0a1(2016-05-18)
You need to log in before you can comment on or make changes to this bug.