Fixes #4673: Fixes BrowserToolbar ripple issues

[sblatz]

---

Old Behavior

New Behavior

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
• Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

[codecov]

Codecov Report

Merging #7301 into master will increase coverage by 0.02%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##             master    #7301      +/-   ##
============================================
+ Coverage     77.35%   77.37%   +0.02%     
  Complexity     5054     5054              
============================================
  Files           674      674              
  Lines         24731    24718      -13     
  Branches       3654     3652       -2     
============================================
- Hits          19130    19126       -4     
+ Misses         4088     4080       -8     
+ Partials       1513     1512       -1     

Impacted Files	Coverage Δ	Complexity Δ
...lla/components/browser/toolbar/edit/EditToolbar.kt	63.63% <33.33%> (ø)	24.00 <0.00> (ø)
...mponents/browser/toolbar/display/DisplayToolbar.kt	78.68% <100.00%> (ø)	47.00 <1.00> (ø)
...mponents/support/migration/TelemetryIdentifiers.kt	0.00% <0.00%> (ø)	0.00% <0.00%> (ø%)
...components/support/sync/telemetry/SyncTelemetry.kt	87.39% <0.00%> (+1.68%)	41.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9473211...dc80037. Read the comment docs.

[mcarare]

@sblatz I've checked your changes against Fenix app, the buttons work as expected, but there are issues with the positioning and a visible "snap" and reordering of items.

The issue with ripple being cut can be solved with adding android:clipChildren="false" to the parent of the view that "hosts" the ripple ( the topmost layout in this case). It will still be cut by the system bar if on some devices the ripple is big enough.

The open tabs button will still be cut by the url view because they are on the same parent.

IMO after aligning the icons and solving that snapping it will still be a big improvement comparing to the current behavior.

LE: The edit-toolbar appears to be broken in this PR.

[pocmo]

Looks like some tests are failing:

[task 2020-06-09T16:42:17.308Z] e: /builds/worker/checkouts/src/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/display/DisplayToolbarTest.kt: (478, 52): Unresolved reference: drawable
[task 2020-06-09T16:42:17.308Z] e: /builds/worker/checkouts/src/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/display/DisplayToolbarTest.kt: (484, 55): Unresolved reference: drawable
[task 2020-06-09T16:42:17.308Z] e: /builds/worker/checkouts/src/components/browser/toolbar/src/test/java/mozilla/components/browser/toolbar/display/DisplayToolbarTest.kt: (488, 52): Unresolved reference: drawable

[sblatz]

@pocmo I'm hopping on those now!

[mcarare]

If you want the ripple to not be cut, this should be on the parent constraint layout.

[mcarare]

Same issue as for display toolbar.

[pocmo]

Some random thoughts:

• When we rewrote the layouts of the toolbar we tried to have a flat layout structure to avoid the performance penalties that come from nested layouts. I'm a bit sad if we need to introduce a nested structure again. :(

• The patch seems to solve the problem for indicators and page actions that live inside the nested layout. But it seems like we still have the problem that browser actions, the menu button or navigation actions (depending on how the toolbar in configured) will be drawn behind the background.

• Nit: Is extending the ripple something we actually want? Clipping them seems to be more in line with desktop at least (although it uses a rectangle shape)

[pocmo]

Two alternative ideas that we could try to fix it for all actions and with keeping the flat structure:

• Replace the views rendering the button with a custom button that renders the ripple effect on the button itself (background or foreground) instead of a parent. But this has the downside that we'd have to change the views of all the actions. And there may be a bunch of custom ones too (e.g. the tab switcher)

• Replace the CoordinatorLayout of the display toolbar with a custom layout extending coordinatorlayout. This layout could turn the background view invisible (so that it does not get drawn, but still gets measured) and then draw the background on itself - before the actual layout background gets drawn (with the ripple). Then the ripple effect would be drawn on top of the display toolbar background and on top of the background.

🤔

[pocmo]

I did try to prototype option 2 and something like this seems to work:

class DisplayToolbarView @JvmOverloads constructor(
    context: Context, attrs: AttributeSet? = null, defStyleAttr: Int = 0
) : ConstraintLayout(context, attrs, defStyleAttr) {
    init {
        // Forcing transparent background so that draw methods will get called and ripple effect
        // for children will be drawn on this layout.
        setBackgroundColor(0x00000000)
    }

    lateinit var backgroundView: ImageView

    override fun onFinishInflate() {
        backgroundView = findViewById(R.id.mozac_browser_toolbar_background)
        backgroundView.visibility = View.INVISIBLE
    }

    // Overrding draw instead of onDraw since we want to draw the background before the actual
    // (transparent) background (with a ripple effect) is drawn.
    override fun draw(canvas: Canvas) {
        canvas.save()
        canvas.translate(backgroundView.x, backgroundView.y)
        backgroundView.drawable.draw(canvas)
        canvas.restore()

        super.draw(canvas)
    }
}

[sblatz]

Closing in favor of @pocmo's solution here: #7323