Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1522: Context menu interfering with text selection menu. #1523

Merged
merged 1 commit into from Sep 10, 2019

Conversation

@iccub
Copy link
Contributor

iccub commented Sep 10, 2019

Summary of Changes

This pull request fixes issue #1522

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).
if (targetLink) {
data.link = targetLink.href;
data.title = targetLink.textContent;

This comment has been minimized.

Copy link
@iccub

iccub Sep 10, 2019

Author Contributor

these title and alt were taken from Firefox


if (!targetLink && !targetImage) {
// No link or image was tapped, sending empty callback so the ContextMenu will know that it
// should not show the menu(only text selection menu if applicable).
webkit.messageHandlers.contextMenuMessageHandler.postMessage(data);

This comment has been minimized.

Copy link
@iccub

iccub Sep 10, 2019

Author Contributor

This added by me, comment explains why

@@ -503,10 +503,6 @@ class Tab: NSObject {
guard let url = self.webView?.url else {
return
}

if let helper = contentScriptManager.getContentScript(ContextMenuHelper.name()) as? ContextMenuHelper {
helper.replaceWebViewLongPress()

This comment has been minimized.

Copy link
@iccub

iccub Sep 10, 2019

Author Contributor

We don't need it anymore, it's added in BVC

@@ -126,6 +125,9 @@ extension ContextMenuHelper: TabContentScript {
guard let data = message.body as? [String: AnyObject] else {
return
}

// No image or link was pressed, we are clearing `elements` so no accidental link tap occurs.
if data.isEmpty { elements = nil }

This comment has been minimized.

Copy link
@iccub

iccub Sep 10, 2019

Author Contributor

the context menu javascript trigger on regular taps too, so if you swipe and touch an image/link by accident, elements was assigned and caused a bug when context menu was showing even if you didn't tap on a link

@iccub iccub merged commit 8141817 into development Sep 10, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@iccub iccub deleted the bugfix/1522 branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.