Closed Bug 1105472 Opened 5 years ago Closed 5 years ago

Domain highlighting fails when the URL is longer than the Awesomebar

Categories

(Firefox for Android :: Awesomescreen, defect, major)

defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 37
Tracking Status
firefox36 --- verified
firefox37 --- verified
fennec + ---

People

(Reporter: gcp, Assigned: mcomella)

References

Details

Attachments

(7 files, 6 obsolete files)

117.77 KB, image/png
Details
141.99 KB, image/png
Details
346.84 KB, image/png
Details
17.93 KB, patch
bnicholson
: review+
Details | Diff | Splinter Review
162.72 KB, image/png
Details
247.64 KB, image/png
Details
8.63 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Attached image Non-highlighted URL
Nexus 7 2013, current m-c.

If the URL of the visited page is longer than what fits in the Awesomebar, the domain highlighting (marking the hostname darker/bolder than the rest of the URL) stops working. This makes phishing easier.
tracking-fennec: --- → ?
Assignee: nobody → michael.l.comella
tracking-fennec: ? → +
Flags: needinfo?(michael.l.comella)
It'd be cool if we could land this for new tablet (soft dependency).
I disabled the fading parts of FadingTextView (by commenting out [1]) and the issue appears to be solved. Looking for a solution...

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/FadedTextView.java?rev=2878bf6cbc1f#79
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
Regression window-wanted:
mozilla-central:
good build: 24-09-2014
bad build: 25-09-2014

pushlog:http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e2993c99323&tochange=1735ff2bb23e
That bisection points at bug 1061508 but that was already somewhat obvious from comment 3 as well.
The host coloring appears to happen here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ToolbarDisplayLayout.java#372

updateGradientShader and FadedTextGradient seem to assume the text only has one color:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/FadedTextView.java#62
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/FadedTextView.java#89

This code dates back to:
http://sriramramani.wordpress.com/2013/06/06/ellip-sis/

note the remark at the end:
"Update: Thanks to Romain Guy! This can be achieved by using android:fadingEdgeLength and android:requiresFadingEdge="horizontal" (in ICS). On pre-ICS phones, it’s enabled using android:fadingEdge. Sigh! So much work for something existing! :("
Thanks for the deep dive, GCP.
Attachment #8536828 - Flags: review?(mhaigh)
To test on a pre-ICS device.
Flags: needinfo?(michael.l.comella)
I presume for the Awesomebar the bad performance effects are much less pronounced?
http://sriramramani.wordpress.com/2013/06/06/ellip-sis/#comment-522
Comment on attachment 8536828 [details] [diff] [review]
Part 1: Change BrowserToolbar to use Android's fading edge

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

::: mobile/android/base/resources/layout/toolbar_display_layout.xml
@@ +32,5 @@
>                                              android:layout_weight="1.0"
> +                                            gecko:autoUpdateTheme="false"
> +                                            android:ellipsize="none"
> +                                            android:singleLine="true"
> +                                            android:fadingEdge="horizontal"

Align all other attributes in this tag with the android:id attribute
Attachment #8536828 - Flags: review?(mhaigh) → review+
Attachment #8536832 - Flags: review?(mhaigh) → review+
Attached image Post-patch: perf graph (obsolete) —
While still maintaining 60 FPS on my N7, this seems like a pretty unacceptable drop in performance for such a inconsequential feature.

Looking into a solution where we draw over the text with a rectangle that uses a linear gradient filter.
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #13)
> Created attachment 8537546 [details]
> Post-patch: perf graph
> 
> While still maintaining 60 FPS on my N7, this seems like a pretty
> unacceptable drop in performance for such a inconsequential feature.

Agreed. Good call.
To give you an idea where I'm going with this.
Note that this does not work for RTL.
Attachment #8543025 - Flags: review?(bnicholson)
Attached image Drawn rectangle size
To show how large the drawn rectangle is compared to font size.
Attachment #8542916 - Attachment is obsolete: true
Attachment #8536828 - Attachment is obsolete: true
Attachment #8536832 - Attachment is obsolete: true
Attachment #8537546 - Attachment is obsolete: true
Discrepencies in perf from comment 19 to comment 12 could also be the number of tabs open, the displayed page content, and (not likely) the text color.
Attachment #8542906 - Flags: review?(margaret.leibovic) → review?(bnicholson)
Comment on attachment 8543025 [details] [diff] [review]
Part 2: Add FadedMultiColorTextView

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

::: mobile/android/base/widget/FadedMultiColorTextView.java
@@ +68,5 @@
> +        final boolean needsNewGradient = (backgroundGradient == null ||
> +                                          backgroundGradient.getBackgroundColor() != backgroundColor ||
> +                                          backgroundGradient.getEndRight() != gradientEndRight);
> +
> +        if (needsEllipsis() && needsNewGradient) {

You already check needsEllipsis() in onDraw(), so drop this to prevent the extra layout calculations.
Attachment #8543025 - Flags: review?(bnicholson) → review+
Attachment #8542906 - Flags: review?(bnicholson) → review+
hg qadd. hg qref. -_-
Attachment #8543039 - Flags: review?(bnicholson)
Attachment #8543025 - Attachment is obsolete: true
(In reply to Brian Nicholson (:bnicholson) from comment #21)
> You already check needsEllipsis() in onDraw(), so drop this to prevent the
> extra layout calculations.

Good call. I chose to pass it in as an arg instead though - keeping the
needsEllipsis check inside updateGradientShader is more future-proof.
Attachment #8543039 - Attachment is obsolete: true
Attachment #8543039 - Flags: review?(bnicholson)
Comment on attachment 8542906 [details] [diff] [review]
Part 1: Make FadedTextView abstract and move current implementation to FadedSingleColorTextView

Note: the reviewer is incorrect on the uploaded version of the patch - please change to bnicholson.

This request is for parts 1 & 2.

Approval Request Comment
[Feature/regressing bug #]:
  Domain highlighting has always had this issue, but it is much more noticeable on new tablet (FF 36) since the url is shown by default.

[User impact if declined]:
  Users with long urls will not have the text highlighted - makes tablet look less polished.

[Describe test coverage new/current, TBPL]:
  None

[Risks and why]: 
  Worst case, we break all of our  TextViews that shorten the text with an ellipsis-technique (we use fading) because part 1 makes the implementing View an abstract super-class and sub-class. More likely, the alternative faded text implementation (part 2; only used in toolbar display text) is underperformant, does not work in certain edge cases (e.g. private browsing), or completely messes with the toolbar text. It closes follows the code paradigm of part 1, however, so I doubt this will be an issue.

[String/UUID change made/needed]: N/A
Attachment #8542906 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7cbfc2f0ee86
https://hg.mozilla.org/mozilla-central/rev/2a4ba57bf69f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Attachment #8542906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in Firefox for Android 37.0a1 (2015-01-05)
Device: Nexus 7 (Android 5.0.1)
Verified as fixed in Firefox for Android 36.0a2 (2015-01-06)
Device: Asus Transformer Pad TF300T (Android 4.2.1)
Depends on: 1120226
I will mark this as Verified fixed based on comment 27 and comment 28.
Status: RESOLVED → VERIFIED
It’s not fixed in 51
I hope you can check this issue.
Thanks and BR.
Flags: needinfo?(mozilla)
lgbrowser5@gmail.com:

If you are experiencing the exact same problem, please open a new bug.

Or is what you are seeing bug 1163963?
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.