Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Description • 10 years ago

Like Bug 1097337, we should aim to match these orange handles too.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 1 • 10 years ago

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

Don't mind me, just leaving this here to remind myself.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 2 • 10 years ago

NI-ing Lucas here to put this on his radar for asset matching and what not.

	Mark Capella [:capella]
	Comment 3 • 9 years ago

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 ?

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 4 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 5 • 9 years ago

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?

	Mark Capella [:capella]
	Comment 6 • 9 years ago

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/

	Mark Capella [:capella]
	Comment 7 • 9 years ago

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?

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 8 • 9 years ago

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

	Mark Capella [:capella]
	Comment 9 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 10 • 9 years ago

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

	Mark Capella [:capella]
	Comment 11 • 9 years ago

cc:ing tylin in case he's interested :)

	Mark Capella [:capella]
	Comment 12 • 9 years ago

Example use

https://www.dropbox.com/home/BugShots?preview=bug1097398_mobileCaretsMods.mp4

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 13 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 14 • 9 years ago

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

	:Margaret Leibovic
	Comment 15 • 9 years ago

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 16 • 9 years ago
important

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 17 • 9 years ago
important

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.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 18 • 9 years ago

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.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 19 • 9 years ago

try these!

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 20 • 9 years ago

Oops! forgot the middle ones.

Can we get a build to test this?

	Mark Capella [:capella]
	Comment 21 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 22 • 9 years ago

Oooops: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2c66c02488a

	Mark Capella [:capella]
	Comment 23 • 9 years ago

I really like this fix for allowing platform specific resources.

	Mark Capella [:capella]
	Comment 24 • 9 years ago

So, waiting for feedback on the build, and assets provided.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 25 • 9 years ago

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

	Mark Capella [:capella]
	Comment 26 • 9 years ago

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 27 • 9 years ago

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?

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 28 • 9 years ago

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

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 29 • 9 years ago

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

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 30 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 31 • 9 years ago

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

	Mark Capella [:capella]
	Comment 32 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 33 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 34 • 9 years ago

(mentioned mcomella couple comments back, noticed he's not cc:ed in, and might like to be)

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 35 • 9 years ago

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

	Mark Capella [:capella]
	Comment 36 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 37 • 9 years ago

Sorry for the spam, I attached the wrong patch :-(

	Mark Capella [:capella]
	Comment 38 • 9 years ago

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

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 39 • 9 years ago

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.

	Mark Capella [:capella]
	Comment 40 • 9 years ago

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

	Mark Capella [:capella]
	Comment 41 • 9 years ago

Looking for feedback on the build provided in comment #38, prior to requesting final code review from snorp

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 42 • 9 years ago

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

	Mark Capella [:capella]
	Comment 43 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 44 • 9 years ago

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

	Mark Capella [:capella]
	Comment 45 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 46 • 9 years ago

(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

	Mark Capella [:capella]
	Comment 47 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 48 • 9 years ago

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

	Mark Capella [:capella]
	Comment 49 • 9 years ago

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?

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 50 • 9 years ago

Can we just adjust for our own use case in the code? I don't think we need to keep this consistent with FFOS.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 51 • 9 years ago

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

	Mark Capella [:capella]
	Comment 52 • 9 years ago

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/

	Mark Capella [:capella]
	Comment 53 • 9 years ago

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/

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 54 • 9 years ago

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

	Mark Capella [:capella]
	Comment 55 • 9 years ago

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.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 56 • 9 years ago

ping Margaret! not sure if you know who might have time to push this over the finish line? :)

	Mark Capella [:capella]
	Comment 57 • 9 years ago

Final version

	Mark Capella [:capella]
	Comment 58 • 9 years ago

Final version.

	:Margaret Leibovic
	Comment 59 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 60 • 9 years ago

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

	:Margaret Leibovic
	Comment 61 • 9 years ago

snorp, can someone from your team help wrap this up?

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 62 • 9 years ago

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

	:Margaret Leibovic
	Comment 63 • 9 years ago

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

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 64 • 9 years ago

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?

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 65 • 9 years ago

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.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 66 • 9 years ago

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

	:Margaret Leibovic
	Comment 67 • 9 years ago

TYLin, would you be interested in picking up this up to help us out?

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 68 • 9 years ago

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.

	:Margaret Leibovic
	Comment 70 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 71 • 9 years ago

I think Margaret might have removed your NI by accident :) let me know when you' have questions though!

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 76 • 9 years ago

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

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 77 • 9 years ago

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

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 78 • 9 years ago

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.

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 79 • 9 years ago

(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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 80 • 9 years ago

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

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 81 • 9 years ago

Comment on attachment 8719718 [details]
MozReview Request: Bug 1097398 Part 2 - Add preferences to make carets always tilt

https://reviewboard.mozilla.org/r/35067/#review31747

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 82 • 9 years ago

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/

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 83 • 9 years ago

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.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 88 • 9 years ago

Hi Anthony, this is the latest build. Again, sorry for the previous broken build.

http://archive.mozilla.org/pub/mobile/try-builds/tlin@mozilla.com-b3d01722929fbf3792cc266c26a429106c0a6bd2/try-android-api-15/fennec-47.0a1.en-US.android-arm.apk

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 89 • 9 years ago

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!

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 90 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 91 • 9 years ago

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

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 92 • 9 years ago

NI-ing for provided assets and comment 91 :)

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 93 • 9 years ago

SVG assets

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 98 • 9 years ago

Latest build with 0.95 opacity assets in comment #92.

http://archive.mozilla.org/pub/mobile/try-builds/tlin@mozilla.com-9b16e265c0ea2ddc4a75975b00b4f9afd54b1010/try-android-api-15/fennec-47.0a1.en-US.android-arm.apk

	Anthony Lam (:antlam) Find me, @antlam on Slack! Reporter
	Comment 99 • 9 years ago

Looks great!

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 100 • 9 years ago

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 101 • 9 years ago

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?

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 102 • 9 years ago

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

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 103 • 9 years ago

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.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 104 • 9 years ago

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.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 105 • 9 years ago

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

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 106 • 9 years ago

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

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 107 • 9 years ago

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

	:Gijs (he/him)
	Comment 108 • 9 years ago

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

	:Gijs (he/him)
	Comment 109 • 9 years ago

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

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 110 • 9 years ago

(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

	:Gijs (he/him)
	Comment 111 • 9 years ago

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

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 112 • 9 years ago

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/

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 113 • 9 years ago

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/

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 114 • 9 years ago

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/

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 115 • 9 years ago

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/

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 116 • 9 years ago

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

	Daniel Holbert [:dholbert]
	Comment 117 • 9 years ago
concern-withdrawn

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

	:Gijs (he/him)
	Comment 118 • 9 years ago

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

	Daniel Holbert [:dholbert]
	Comment 119 • 9 years ago

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

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 120 • 9 years ago

BTW, the images are used in desktop build for marionette test and some reftest-style mochitest.

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 121 • 9 years ago

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.

	Cameron McCormack (:heycam)
	Comment 122 • 9 years ago

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 123 • 9 years ago

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 :)

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 124 • 9 years ago

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.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 125 • 9 years ago

Removing NI -- review is looking good.  Thanks Gijs, dholbert, heycam, (others).

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 126 • 9 years ago

> Further to heycam -- consider filing a mentor ticket for these other
> resources.  A good first bug, I think :)

I filed bug 1252368 for this :)

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 127 • 9 years ago

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

Nick, thank you for the review!

	Pulsebot
	Comment 128 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeebbe5ee8fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0be9a37a515
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe725e70124
https://hg.mozilla.org/integration/mozilla-inbound/rev/309cc0dac51c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 129 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/eeebbe5ee8fb
https://hg.mozilla.org/mozilla-central/rev/d0be9a37a515
https://hg.mozilla.org/mozilla-central/rev/2fe725e70124
https://hg.mozilla.org/mozilla-central/rev/309cc0dac51c

	Alex Caziuc
	Comment 130 • 8 years ago

Tested using:
Device: Huawei Honor (Android 5.1.1)
Build: Firefox for Android Nightly 49.0a1(2016-05-18)