Activate vomnibar in main/top-level window only (WIP) #1537

smblott-github · 2015-03-16T10:37:43Z

This changes vomnibar commands to activate not in the current frame, but in window.top. Consequently, the vomnibar always appears in the same position, and we don't get odd looking vomnibars in small frames.

Apart from the better UX, this seems to be the right thing to do. Vomnibar commands apply to tabs (not frames).

Currently incomplete:

~~On exit, the focus is not returned to the frame which originally had the focus. (It's returned to window.top).~~ Fixed.

@mrmr1993. What do you think? Have you any ideas how to get the focus back to the original frame?

This changes vomnibar commands to activate not in the current frame, but in window.top. Consequently, the vomnibar always appears in the same position, and we don't get odd looking vomnibars in small frames. Apart from the better UX, this seems to be the right thing to do. Vomnibar commands apply to tabs (not frames). Currently incomplete: - On exit, the focus is not returned to the frame which originally had the focus. (It's returned to window.top). - The vomnibar can lose the focus and not hide itself for various frame/click combinations.

mrmr1993 · 2015-03-16T14:57:52Z

@smblott-github this causes the Vomnibar to fail when the top level frame has a <frameset> rather than a <body>.

Have you any ideas how to get the focus back to the original frame?

I think frameIdsForTab[tab.id] should still be intact (in the background page) when exiting the Vomnibar, so we can message the last focused frame to tell it to refocus itself. Unfortunately, this involves a round trip to the background page.

I think (but haven't tested that) it also might be possible to entangle a port with the Vomnibar frame by doing

vomnibarUrl = chrome.runtime.getURL "pages/vomnibar.html"
foundVomnibarFrame = false
for frameWindow in window.top
  try # Catch errors from accessing location on cross-origin frames.
    if frameWindow.location == vomnibarUrl
      foundVomnibarFrame = true
      # Set up a port with the window here.
      break
unless foundVomnibarFrame
  # Code to mitigate there being no such frame.

which would avoid the round trip to the background page, by having the vomnibar communicate with the opener frame directly.

smblott-github · 2015-03-17T09:30:28Z

this causes the Vomnibar to fail when the top level frame has a rather than a

Actually, not. I've been using this as the main test page, and it works fine there.

I think frameIdsForTab[tab.id] should still be intact

It turns out to be better/easier than that. The request to open the vomnibar contains the frameId of the originating frame. So the top window knows which frame to focus when it's done.

The basics are now functional (efada88). For example, opening the vomnibar in the hangouts sidebar opens a full-window vomnibar, and returns the focus to the sidebar when the vomnibar closes.

The approach I took involves extra massaging when the vomnibar closes, but no more messaging than when the vomnibar opened. So no big deal.

To do: previously, we hid the vomnibar when the host frame receives the focus. Now, we probably need to hide the vomnibar when any other frame receives the focus. Not sure. Requires more thought.

mrmr1993 · 2015-03-17T13:38:04Z

Actually, not.

Sorry! That was a problem that I ran into before, something must have changed (presumably VIF).

Now, we probably need to hide the vomnibar when any other frame receives the focus.

This seems like the most sensible choice. We can easily do this by sending a vomnibar-specific close message in handleFrameFocused.

smblott-github · 2015-03-17T14:00:13Z

We can easily do this by sending a vomnibar-specific close message in handleFrameFocused.

It's actually implemented now. I could describe the parts individually, but it'd probably be better if you just review the code (which would be appreciated, if you're able @mrmr1993).

Summary:

Vomnibar activation is only handled in the top frame. However, the request contains the frameId of the original frame. This is passed to the vomnibar so that we can refocus that frame later.
The main/top frame is focused and the vomnibar operates there.
When the vomnibar closes, we use a new background handler (sendMessageToFrames) to send a focusFrame request back to the original frame.
However, there's a subtlety. When the vomnibar closes, Chrome focuses the containing frame. There's therefore a race condition as to which frame ends up focused. To get around this, we wait until the containing frame receives the focus before sending the focusFrame message.
Whenever any frame receives the focus, we now broadcast a frameFocused message to all frames. This is the same as @mrmr1993's idea, above. The vomnibar receives this and, if it's active, then it hides itself.

UX decision:

Currently, when the vomnibar hides, we flash the original frame, but only if it isn't the main/top frame. This looks OK to me, but may not be necessary.

mrmr1993 · 2015-03-17T14:04:08Z

background_scripts/main.coffee

@@ -657,6 +657,19 @@ handleFrameFocused = (request, sender) ->
  if frameIdsForTab[tabId]?
    frameIdsForTab[tabId] =
      [request.frameId, (frameIdsForTab[tabId].filter (id) -> id != request.frameId)...]
+  console.log frameIdsForTab[tabId]


This should be removed

@mrmr1993

Clean up, and fixes following code review from @mrmr1993.

mrmr1993 · 2015-03-17T14:38:23Z

content_scripts/vimium_frontend.coffee

+  if request.command.split(".")[0] == "Vomnibar"
+    if DomUtils.isTopFrame()
+      # We pass the frameId from request.  That's the frame which originated the request, so that's the frame
+      # which whould receive the focus when the vomnibar closes.


Typo; whould should be should.

smblott-github · 2015-03-18T06:52:48Z

A reason for not doing this is...

If Vimium is disabled for the main/top frame, but enabled for some other frame, then the vomnibar doesn't work.

How important is that? Not sure. On the one hand, it's not a common case. On the other, it's pretty broken.

Imagine the status quo was vomnibar-in-main-frame and someone proposed getting around this quirk by moving the vomnibar to the host frame (with strange, small vomnibars). Would we buy that? Not sure. Probably not.

mrmr1993 · 2015-03-18T11:57:55Z

The most logical fix I can think of is to execute Vomnibar.init when one or more frames are active and Vomnibar.deinit (doesn't exist, but basically just has to remove the Vomnibar iframe from the document) when none are active. Obviously this wants to be repeated every time exclusion rules are updated. Checking whether any frames are active is easy (in the background) using chrome.webNavigation.getAllFrames and checking the urls of all the frames.

smblott-github · 2015-04-18T15:05:26Z

Update. Having used this for some time, it seems to be robust and work well. To summarise, when the vomnibar opens within frames, we get something like this:

With this PR, the vomnibar is always in the main frame, always in the same place, always the same size and always visible.

The down side, however, is that if Vimium is disabled in the main frame but active in the active frame, then the vomnibar doesn't work. That's pretty serious. However, I've yet to come across a live example of this. One case would be Google's Inbox with its thin and tall Hangouts frame. However, there, the vomnibar is useless currently (because it's not visible at all anyway). Edit. Now fixed.

~~Nevertheless, if we were to merge this PR, it would be a good candidate for Vimium Labs. So I've put it together as "Vimium Labs" setting on the options page:~~ Edit. Now removed. No longer necessary because the Vomnibar is now always activatable in the main/top frame, even if Vimium is disabled there. Screenshot removed.

Vomnibar commands are handled in a frame even is isEnabledForUrl is false.

Since the vomnibar now always is initialised and opened the top window (see d673474), there's no longer a need for this to be a Vimium Labs feature.

smblott-github · 2015-04-19T07:03:46Z

Another update. With d673474, the Vomnibar now activates in the top/main frame even if Vimium is disabled in that frame. As a consequence, there's no longer a need to consider this as a "Vimium Labs" feature.

smblott-github · 2015-04-21T14:23:55Z

Status:

Seems to work nicely. (I've been using this for quite some weeks.)

Issues...

If the current and main frames are the same: works fine.
If the Vimium is enabled in both the current and the main frame: works fine.
If Vimium is not enabled in the main frame: works fine.
If Vimium cannot run in the main frame (e.g. it's a chrome extension page): broken. How important is this last case? I haven't come across it in the wild. Are there any new-tab extensions, for example, which embed frames on which Vimium can run?

Another issue: ~~There's sometimes a slight Vomnibar flicker when returning to a tab after previously using the Vomnibar to open a link in a new tab.~~ (Edit: Fixed.)

philc · 2015-04-21T21:27:43Z

This LGTM. The 4th case you mention sounds incredibly niche to me.
On Apr 21, 2015 7:23 AM, "Stephen Blott" notifications@github.com wrote:

Status:

Seems to work nicely. (I've been using this for quite some weeks.)

Issues...

If the current and main frames are the same: works fine.

If the Vimium is enabled in both the current and the main frame:
works fine.

If Vimium is not enabled in the main frame: works fine.

If Vimium cannot run in the main frame (e.g. it's a chrome
extension page): broken. How important is this last case? I haven't come
across it in the wild. Are there any new-tab extensions, for example, which
embed frames on which Vimium can run?

Another issue: There's sometimes a slight Vomnibar flicker when returning
to a tab after previously using the Vomnibar to open a link in a new tab.

—
Reply to this email directly or view it on GitHub
#1537 (comment).

Remove reference to unused setting "vomnibarInTopFrame".

1. Rework event handling to eliminate frame flicker (a la philc#1485). 2. Tidy up logic. Which should make this more robust.

smblott-github · 2015-04-23T10:22:44Z

More update. The Vomnibar flicker issue has been fixed.

Opinion: We should merge this. It's a big UX win.

I'll merge soon, unless there's significant push back.

Activate vomnibar in window.top; refocus original frame.

efada88

smblott-github added 2 commits March 17, 2015 09:34

Activate vomnibar in window.top; minor clean up.

e65f4d4

Activate vomnibar in window.top; simplify messaging.

aa8d00f

smblott-github added the smb-watching label Mar 17, 2015

smblott-github added 4 commits March 17, 2015 10:32

Activate vomnibar in window.top; hide on focus.

9461d30

Activate vomnibar in window.top; hide on focus, fixed.

8bc811a

Activate vomnibar in window.top; more clean up.

4399e2e

Activate vomnibar in window.top; fix race condition on close.

71d33fd

mrmr1993 reviewed Mar 17, 2015
View reviewed changes

Activate vomnibar in window.top; more clean up.

b65f285

Clean up, and fixes following code review from @mrmr1993.

mrmr1993 reviewed Mar 17, 2015
View reviewed changes

smblott-github added 2 commits March 17, 2015 15:08

Activate vomnibar in window.top; yet more clean up.

8d09dc1

Activate vomnibar in window.top; fix dropped line.

49c5204

smblott-github mentioned this pull request Mar 21, 2015

Vimium Labs (add "experimental" section to options page) #1542

Closed

Activate vomnibar in window.top; add "labs" option.

a469e82

smblott-github added 2 commits April 19, 2015 07:43

Activate vomnibar in window.top; handle Vomnibar even if disabled.

d673474

Vomnibar commands are handled in a frame even is isEnabledForUrl is false.

Activate vomnibar in window.top; strip Vimium Labs.

5b0036a

Since the vomnibar now always is initialised and opened the top window (see d673474), there's no longer a need for this to be a Vimium Labs feature.

smblott-github added 2 commits April 19, 2015 08:07

Activate vomnibar in window.top; strip (now) unnecessary setting.

43b5501

Activate vomnibar in window.top; fix logic error.

2ed990d

smblott-github added 2 commits April 22, 2015 15:03

Tidy up.

9eaa733

Remove reference to unused setting "vomnibarInTopFrame".

Activate vomnibar in window.top; no flicker and tidy up.

bfa6c88

1. Rework event handling to eliminate frame flicker (a la philc#1485). 2. Tidy up logic. Which should make this more robust.

smblott-github merged commit bfa6c88 into philc:master Apr 25, 2015

smblott-github deleted the vomnibar-in-main-window branch February 21, 2016 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Activate vomnibar in main/top-level window only (WIP) #1537

Activate vomnibar in main/top-level window only (WIP) #1537

Uh oh!

smblott-github commented Mar 16, 2015

Uh oh!

mrmr1993 commented Mar 16, 2015

Uh oh!

smblott-github commented Mar 17, 2015

Uh oh!

mrmr1993 commented Mar 17, 2015

Uh oh!

smblott-github commented Mar 17, 2015

Uh oh!

mrmr1993 Mar 17, 2015

Uh oh!

mrmr1993 Mar 17, 2015

Uh oh!

smblott-github commented Mar 18, 2015

Uh oh!

mrmr1993 commented Mar 18, 2015

Uh oh!

smblott-github commented Apr 18, 2015

Uh oh!

smblott-github commented Apr 19, 2015

Uh oh!

smblott-github commented Apr 21, 2015

Uh oh!

philc commented Apr 21, 2015

Uh oh!

smblott-github commented Apr 23, 2015

Uh oh!

Uh oh!

Activate vomnibar in main/top-level window only (WIP) #1537

Activate vomnibar in main/top-level window only (WIP) #1537

Uh oh!

Conversation

smblott-github commented Mar 16, 2015

Uh oh!

mrmr1993 commented Mar 16, 2015

Uh oh!

smblott-github commented Mar 17, 2015

Uh oh!

mrmr1993 commented Mar 17, 2015

Uh oh!

smblott-github commented Mar 17, 2015

Uh oh!

mrmr1993 Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

mrmr1993 Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

smblott-github commented Mar 18, 2015

Uh oh!

mrmr1993 commented Mar 18, 2015

Uh oh!

smblott-github commented Apr 18, 2015

Uh oh!

smblott-github commented Apr 19, 2015

Uh oh!

smblott-github commented Apr 21, 2015

Uh oh!

philc commented Apr 21, 2015

Uh oh!

smblott-github commented Apr 23, 2015

Uh oh!

Uh oh!