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

For #10555 - Dont auto show toolbar after short taps #10562

Merged
merged 4 commits into from Jul 16, 2021
Merged

For #10555 - Dont auto show toolbar after short taps #10562

merged 4 commits into from Jul 16, 2021

Conversation

Mugurell
Copy link
Contributor

@Mugurell Mugurell commented Jul 6, 2021

We need to wait until having a response from GeckoView on how it handled the
touch only after which we'll know whether to animate the toolbar or not.

The edgecase scenario of having pull to refresh enabled even before having a
response from GeckoView will still work because "canOverscrollTop()" only
checks for the touch to not be handled by the browser to pan the page.

ShortTapsAreIgnored.mp4

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.

@mergify
Copy link
Contributor

mergify bot commented Jul 7, 2021

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

@Mugurell Mugurell requested a review from Amejia481 as a code owner July 8, 2021 10:37
@mergify
Copy link
Contributor

mergify bot commented Jul 8, 2021

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

@pocmo pocmo self-assigned this Jul 9, 2021
@Mugurell
Copy link
Contributor Author

Thank you @pocmo !

I see one test was failing, because here I haven't updated it based on the proposed changes - inputResult is not unhandled but unknown.

Everything should be green now but even if the changes are small I'd prefer landing this after the merge and after some time in Nightly uplift it if needed.

@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2021

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

@Mugurell
Copy link
Contributor Author 

�[36mINFO�[0m[0062] Running: [/bin/sh -c URL_FLANK_BIN="$($CURL --silent 'https://api.github.com/repos/Flank/flank/releases/latest' | jq -r '.assets[].browser_download_url')"     && $CURL --output "${TEST_TOOLS}/flank.jar" "${URL_FLANK_BIN}"     && chmod +x "${TEST_TOOLS}/flank.jar"] 
curl: (3) Illegal characters found in URL
error building image: error building stage: failed to execute command: waiting for process to exit: exit status 3
Error: Could not build image.
[taskcluster 2021-07-14 16:23:57.527Z] === Task Finished ===
[taskcluster 2021-07-14 16:23:57.596Z] Artifact "public/image.tar.zst" not found at "/workspace/image.tar.zst"
[taskcluster 2021-07-14 16:23:57.664Z] Unsuccessful task run with exit code: 1 completed in 71.923 seconds

One more try.

@Mugurell Mugurell closed this Jul 14, 2021
@Mugurell Mugurell reopened this Jul 14, 2021
@Mugurell
For mozilla-mobile#10555 - Use INPUT_HANDLING_UNKNOWN as the default …
9891d33 
…of InputResultDetail

We need to wait until having a response from GeckoView on how it handled the
touch only after which we'll know whether to animate the toolbar or not.

The edgecase scenario of having pull to refresh enabled even before having a
response from GeckoView will still work because "canOverscrollTop()" only
checks for the touch to not be handled by the browser to pan the page.
@Mugurell
For mozilla-mobile#10555 - Fix InputResultDetail hashcode() collisions
f9e0907 
The previous implementation was affected by the fact that the class has 3
integer properties, all with values [0..4] (-1 was added recently) and it
would just add those value to compute the hashcode.
Collisions were a given.

By using decimal places for each property the new implementation should avoid
collisions while allowing for all the other expected guarantees.
@Mugurell
For mozilla-mobile#10555 - BrowserToolbarBehaviorTest cleanup
a6f47fd 
This commented method seems to be a leftover from the previous refactoring.
"behavior.forceExpand(..)" now calls "expandWithAnimation" for which we already
have a test at line 450.
@Mugurell
Copy link
Contributor Author

I'm seeing the same error as above for the build-docker-image-ui-tests task.
Rebasing and trying again.

@Mugurell Mugurell added the 🛬 needs landing PRs that are ready to land label Jul 16, 2021
@mergify mergify bot merged commit 01ca80d into mozilla-mobile:master Jul 16, 2021
@Mugurell Mugurell deleted the dontAutoShowToolbar branch August 2, 2021 10:50
This was referenced Aug 2, 2021
Closed
Closed
@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

@Mergifyio backport releases_v91.0.0

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command backport releases_v91.0.0: failure

No backport have been created

  • Backport to branch releases_v91.0.0 failed: Branch not found

@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

@Mergifyio backport releases_91.0

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command backport releases_91.0: failure

No backport have been created

  • Backport to branch releases_91.0 failed: Branch not found

@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

@Mergifyio backport releases/91.0

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command backport releases/91.0: failure

No backport have been created

  • Backport to branch releases/91.0 failed

@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

@Mergifyio backport releases/91.0

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command backport releases/91.0: failure

No backport have been created

  • Backport to branch releases/91.0 failed

@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

Not sure why this fails.
Hoping that if works now after some time left to rest.

@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

@Mergifyio backport releases/91.0

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command backport releases/91.0: failure

No backport have been created

  • Backport to branch releases/91.0 failed

@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 2, 2021

@Mergifyio backport releases/91.0

@mergify
Copy link
Contributor

mergify bot commented Aug 2, 2021

Command backport releases/91.0: failure

No backport have been created

  • Backport to branch releases/91.0 failed

@Mugurell Mugurell mentioned this pull request Aug 2, 2021
Closed
@Mugurell
Copy link
Contributor Author

Mugurell commented Aug 3, 2021

@Mergifyio backport releases/91.0

@mergify
Copy link
Contributor

mergify bot commented Aug 3, 2021

Command backport releases/91.0: failure

No backport have been created

  • Backport to branch releases/91.0 failed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pocmo pocmo pocmo approved these changes

@csadilek csadilek Awaiting requested review from csadilek

@Amejia481 Amejia481 Awaiting requested review from Amejia481

Assignees

@pocmo pocmo

Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Mugurell @pocmo