Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add toolbarVisible getter to WebAppHideToolbarFeature #7216

Closed
wants to merge 1 commit into from

Conversation

NotWoods
Copy link
Contributor

@NotWoods NotWoods commented Jun 2, 2020

Exposing this to fix the toolbar issues in Fenix. mozilla-mobile/fenix#10452


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
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #7216 into master will decrease coverage by 0.03%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7216      +/-   ##
============================================
- Coverage     77.24%   77.21%   -0.04%     
- Complexity     4954     4973      +19     
============================================
  Files           660      657       -3     
  Lines         24326    24421      +95     
  Branches       3575     3581       +6     
============================================
+ Hits          18790    18856      +66     
- Misses         4063     4070       +7     
- Partials       1473     1495      +22     
Impacted Files Coverage Δ Complexity Δ
...ts/feature/pwa/feature/WebAppHideToolbarFeature.kt 65.00% <77.77%> (+3.88%) 9.00 <4.00> (+1.00)
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100.00% <0.00%> (ø) 11.00% <0.00%> (ø%)
...wser/menu2/adapter/SmallMenuCandidateViewHolder.kt
...rowser/menu2/adapter/RowMenuCandidateViewHolder.kt
...nts/browser/menu2/adapter/icons/MenuIconAdapter.kt
...onents/browser/menu2/adapter/LastItemViewHolder.kt
...s/browser/menu2/adapter/MenuCandidateViewHolder.kt
...telemetry/measurement/ExperimentsMapMeasurement.kt
.../java/org/mozilla/telemetry/net/TelemetryClient.kt
...er/menu2/adapter/DividerMenuCandidateViewHolder.kt
... and 24 more

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 0bd5f83...994e04c. Read the comment docs.

@jonalmeida
Copy link
Contributor

Please add an issue as reference so we know which bug this is fixing. 🙂

@NotWoods
Copy link
Contributor Author

NotWoods commented Jun 2, 2020

Added mozilla-mobile/fenix#10452 to the description

Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see the fenix side of this as well. To me this looks like an ergonomic change. I.e. without this change, in fenix can't you already query visibility directly via the the toolbar view, toolbar.visibility.

Or do you not have easy access to toolbar where you'd need it?

@NotWoods
Copy link
Contributor Author

NotWoods commented Jun 3, 2020

Fenix sets the visibility field in other places, so this is used to query what the feature thinks the toolbar visibility should be, rather than what it is.

@grigoryk
Copy link
Contributor

grigoryk commented Jun 3, 2020

Fenix sets the visibility field in other places, so this is used to query what the feature thinks the toolbar visibility should be, rather than what it is.

That just sounds like a recipe for other bugs to crop up. Can we have a single point of responsibility for controlling toolbar visibility?

@NotWoods
Copy link
Contributor Author

NotWoods commented Jun 4, 2020

I think this is best for a quick fix (just requires an extra if statment in Fenix), and I'll work on a follow up to move most of this logic into BrowserStore. Eventually we can have something along the lines of:

fun SessionState.hideToolbar() = content.fullscreen || content.pictureInPicture || content.url.isInScope()

@NotWoods
Copy link
Contributor Author

NotWoods commented Jun 4, 2020

I've set up an alternative BrowserStore based solution here: #7251

@grigoryk
Copy link
Contributor

We can close this, right? @NotWoods

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants