1059450 - Regression: Thin pixel bar present next to the back button on tablets in the new toolbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Screen Shot 2014-08-27 at 2.51.56 PM.png

See screenshot.

--
Sony Xperia Z (Nightly 08/27)

Comment 1

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

On long-tap, the bar extends beyond the border of the address-bar

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #1)
> Created attachment 8480051 [details]
> screenshot.png
> 
> On long-tap, the bar extends beyond the border of the address-bar

Is this related to this bug?

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Fix shape dimention in BackButton (r=mcomella)

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8480530 [details] [diff] [review]
Fix shape dimention in BackButton (r=mcomella)

This was a thinko of mine. The button shape path should fill the whole view bounds and the border stroke is painted half in & half out of the path.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8480530 [details] [diff] [review]
Fix shape dimention in BackButton (r=mcomella)

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

I'm not sure how this fixes the issue - can you elaborate on why this draws a line next to the back button?

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8480530 [details] [diff] [review]
Fix shape dimention in BackButton (r=mcomella)

(In reply to Michael Comella (:mcomella) from comment #5)
> Comment on attachment 8480530 [details] [diff] [review]
> Fix shape dimention in BackButton (r=mcomella)
> 
> Review of attachment 8480530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure how this fixes the issue - can you elaborate on why this draws
> a line next to the back button?

Apologies, I should have given more context. The line next to the back button is an artifact caused by the difference between the view and shape bounds. If the shape is smaller than the view, the saveLayer() call will capture stuff that is 'outside' the shape. See:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/CanvasDelegate.java#41

Another way to fix this could be to use the shape dimensions here:
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BackButton.java#60

But I find it cleaner to simply have the shape matching the view bounds.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8480530 [details] [diff] [review]
Fix shape dimention in BackButton (r=mcomella)

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

r+, to avoid the lengthy review back-and-forth cycle, but I still have questions.

If we're drawing a circle to the view bounds, how does this eliminate a line from the View where the circle is not drawn (e.g. top-right corner, outside the boundaries of the circle)? Does the circle draw transparency over the smallest rect containing the circle where the circle is not visible?

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #7)
> Comment on attachment 8480530 [details] [diff] [review]
> Fix shape dimention in BackButton (r=mcomella)
> 
> Review of attachment 8480530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+, to avoid the lengthy review back-and-forth cycle, but I still have
> questions.
> 
> If we're drawing a circle to the view bounds, how does this eliminate a line
> from the View where the circle is not drawn (e.g. top-right corner, outside
> the boundaries of the circle)? Does the circle draw transparency over the
> smallest rect containing the circle where the circle is not visible?

Pretty much. The line is simply a region of the view that is not 'reached' by the alpha mask defined in ShapedButton's mPath.

Comment 9

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/dbedb33e649d

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dbedb33e649d