Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

reliably activate titleMode based on mouse state #9411

Merged
merged 1 commit into from
Jun 28, 2017
Merged

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jun 13, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Auditors: @bbondy, @bsclifton
fix #9410

Test Plan:

General manual test plan is to:

Check when mouse is over navigation content, titleMode should be active. Otherwise it should not.

Also shortcuts such as cmd/ctrl+L and f6 should activate urlbar even when mouse is not over navigation content.

Below specific steps for bugs found in different platforms.

on macOS

  1. Have the window not maximized
  2. Move mouse over navigation content.
  3. urlbar is active, that's ok
  4. Move mouse out of window
  5. titleMode should be active (urlbar should be hidden)

on Windows

  1. Have the window not maximized
  2. Move mouse over navigation content.
  3. urlbar is active, that's ok
  4. Move mouse out of window
  5. titleMode is active, that's ok
  6. Move mouse from top of the window up to website's title
  7. titleMode should be removed (urlbar should be active)

alert active

  1. Open jsFiddle.net and type under js console alert()
  2. Alert is open
  3. titleMode should be active (no visible urlbar)

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@@ -750,8 +750,6 @@ class Frame extends React.Component {
if (!this.frame.isEmpty()) {
windowActions.setNavigated(e.url, this.props.frameKey, false, this.props.tabId)
}
// force temporary url display for tabnapping protection
windowActions.setMouseInTitlebar(true)
Copy link
Contributor Author

@cezaraugusto cezaraugusto Jun 15, 2017

Choose a reason for hiding this comment

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

I tested both with and without this forced display and as far as I can tell no visual changes between them. In both cases, setting page to auto-refresh removes titleMode temporarily and letting tab idle while navigating other tab didn't make any action. Please let me know if I should test another scenario for tabnapping.

@cezaraugusto
Copy link
Contributor Author

btw I also confirmed with @bradleyrichter that titleMode should be always on when mouse is over navigation chrome and aways off when it is not. Currently, it is off when you move your mouse out of the window going all way top. This is not expected and was removed in this PR.

@bradleyrichter
Copy link
Contributor

@cezaraugusto I think you mean the inverse. When the mouse is in the title/search bar area, titlemode should be OFF and URL search bar mode should be ON. When the mouse exits the URL bar area, titlemode should return to on.

@bsclifton
Copy link
Member

before I forget: we'll want to test this logic when an alert is open (because an alert forces title mode)

@cezaraugusto
Copy link
Contributor Author

@bsclifton ok I updated STR with that, seems working as expected

Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

test plan works. LGTM.

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes work great 😄

There is something I noticed which will need a follow up- I'll create an issue and share here

@bsclifton bsclifton merged commit de20bc7 into master Jun 28, 2017
@bsclifton bsclifton deleted the urlbar/9410 branch June 28, 2017 18:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

titleMode does not activate reliably
4 participants