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

Unable to expand groups and open emails on inbox.google.com with f #1307

Closed
maximbaz opened this issue Dec 14, 2014 · 31 comments
Closed

Unable to expand groups and open emails on inbox.google.com with f #1307

maximbaz opened this issue Dec 14, 2014 · 31 comments

Comments

@maximbaz
Copy link

Suppose I have a group of conversations on Google Inbox:

image

I want to expand it, so I hit f:

image

Finally I press K.

Expected: Group is expanded (like on the picture below)
Actual: Group remains collapsed (just like on the first picture)

image

Same happens not only with groups, but with emails as well:

image

Nothing happens when I hit E.


Kubuntu 14.10, Google Chrome 39.0.2171.71
b6125a2
I have no custom rules defined in vimium options.

@mrmr1993
Copy link
Contributor

This sounds suspiciously like #1091, but I don't have google inbox to test that theory. Can you try merging #1167 and see if that helps?

@smblott-github
Copy link
Collaborator

Confirmed. Also, unfortunately #1167 doesn't fix it.

@smblott-github
Copy link
Collaborator

I've only gotten so far as to establish that the selected element is not responding to DomUtils.simulateClick().

@smblott-github
Copy link
Collaborator

Off topic (and not a solution to this problem)...

inbox.google.com has good keyboard shortcuts of its own for the basic operations. When the new release emerges (shortly), you can mix and match inbox's shortcuts with Vimium's. These are the inbox shortcuts I'm using:

snapshot

Other keys (like O, B, etc) are handled by Vimium.

@maximbaz
Copy link
Author

True, another approach is to switch into insert mode and use native shortcuts, as we discussed in another thread.
I like the fact that we have many options, still agree with you that this particular issue has to be fixed anyway.

@mrmr1993
Copy link
Contributor

I've only gotten so far as to establish that the selected element is not responding to DomUtils.simulateClick().

Does the element with the link hint have an event listener?

@smblott-github
Copy link
Collaborator

Does the element with the link hint have an event listener?

Not sure. Unfortunately, I don't really know how to debug this kind of thing. This is your field of expertise, @mrmr1993.

Here's the element:

snapshot

@mrmr1993
Copy link
Contributor

This is your field of expertise, @mrmr1993.

We show a link hint for it because it has tabIndex="0". You can check event listeners in the developer tools tab, but if I'm to be the one debugging this, I'd better just give you a script to extract the information:

var eventListeners = [], $0Children = [].slice.call($0.getElementsByTagName("*"));
$0Children.push($0);
for (var i=0, l=$0Children.length;i<l;i++) {
  var el = $0Children[i], eventListenersForEl = getEventListeners(el), hasEventListeners = false, eventListenerStrings = {};
  for (var eventType in eventListenersForEl) {
    hasEventListeners = true;
    eventListenerStrings[eventType] = Array.prototype.map.call(eventListenersForEl[eventType], (function(handlerObj){
      return {
        "listener": handlerObj.listener.toString(),
        "type": handlerObj.type,
        "useCapture": handlerObj.useCapture
      };
    }));
  }
  if (hasEventListeners) {
    eventListeners.push({
      "elementTag": el.outerHTML.replace(el.innerHTML, ""),
      "eventListeners": eventListenerStrings
    });
  }
}
copy(eventListeners);

@smblott-github could you select that element again in developer tools and run this in the console? It will copy a JSON object listing all the event listeners of the selected element and its children to the clipboard. Hopefully there shouldn't be anything sensitive included, but obviously redact it if there is :)

@smblott-github
Copy link
Collaborator

Thanks, @mrmr1993.

The console:

snapshot

And the clipboard:

[]

Redacting sensitive information proved unnecessary!

It looks like all of the onclick handlers are on document or body:

snapshot

(When you open any of these handlers, you again see that these are attached to document or body.)

@mrmr1993
Copy link
Contributor

Ahh, I hate it when they do this. IMDb does the same for certain UI elements.

I'll try some stuff out with IMDb, and if I have any success hopefully it'll transfer easily to Inbox.

@maximbaz
Copy link
Author

if I'm to be the one debugging this ...

@mrmr1993, I can give you an Inbox invite if you need one. If you also need a device to activate it, just create a temporary account, that you would use for development, and I will activate it later today (probably in the evening).

@mrmr1993
Copy link
Contributor

I can give you an Inbox invite if you need one. If you also need a device to activate it

@z0rch I don't have a device to activate it on, so that would be amazing! I'll send the account details to the email address on your profile :)

@smblott-github
Copy link
Collaborator

I'll send the account details to the email address on your profile

Copy me ([email protected]). I can try to activate it now.

If you also need a device to activate it...

Great thinking, @z0rch.

@mrmr1993
Copy link
Contributor

Email sent. Thanks you guys!

@smblott-github
Copy link
Collaborator

@mrmr1993. Do you need to verify/validate the account or something?

snapshot

(User name and password were copy-and-paste (and look right)).

Also, I tried sending you an invite...

snapshot

@mrmr1993
Copy link
Contributor

Weird, it kicked me out of gmail and made me verify the account with a phone number. I presume that's because you logged in! It should work now.

@smblott-github
Copy link
Collaborator

OK, @mrmr1993. You should be good to go: https://inbox.google.com/

@mrmr1993
Copy link
Contributor

You should be good to go

Fab, thanks! :D :D

@mrmr1993
Copy link
Contributor

It uses the Google JSAction library. I've pushed PR #1316 to enable link hints for click event listeners registered with jsaction.

The link hints overlap in inbox, so we need some way of

  1. identifying which hint is for which element; and
  2. separating the elements.

This is a hard problem.

@smblott-github
Copy link
Collaborator

Excellent work, @mrmr1993.

The link hints overlap in inbox

Like this...

snapshot

So, with #1316, we're selecting two elements and two link hints:

The problem would be solved (would it not?) if we had a reason for filtering out the first of these. Can we detect that it won't respond to simulateClick?

@mrmr1993
Copy link
Contributor

The problem would be solved (would it not?) if we had a reason for filtering out the first of these.

Unfortunately not; for me the link hint to click the circle (it acts as a checkbox) lies on top of the one for expanding the group. We should definitely be showing both of them, so this is an issue we'll have to resolve.

This also happens on the "Add a subscription" button on theoldreader.com, so we can use this as another test case.

@smblott-github
Copy link
Collaborator

Might #1252 help?

@mrmr1993
Copy link
Contributor

Not really; since they're both in a very similar position, it's almost impossible to tell which activates which element even as we flick through them.

@smblott-github
Copy link
Collaborator

OK.

So, we have two problems:

  1. We're activating link hints for elements which don't respond to clicks (just because tabIndex is set).
  2. There are clickable elements which overlap (in this case, exactly), and the user can't possibly know which hint refers to which element.

For problem 1: Why are we activating these hints? Anything which we can actually click will be selected by one of the other XPath terms (will it not?). So we can solve this by just removing the tabIndex condition from the XPath. (Would that be safe?)

We can see problem 2 here:

snapshot

20 is under 21, 22 is under 23, and so on. And the user can't know which is which. So we need a rule to pick one. For example, if one is an ancestor of the other, then pick the ancestor. Or, if one is larger than the other, pick the larger.

In this particular example (but not necessarily in all examples), either of those rules would do the right thing (I assume that opening the message is the right thing).

(I've been wondering for some time if we need a file `hacks.coffee` for sites which don't play nicely. But I couldn't possibly say that out loud.)

@mrmr1993
Copy link
Contributor

We're activating link hints for elements which don't respond to clicks

For tabindex elements I believe one of the following applies:

  • the element is inherently focusable (eg. <input>/<textarea>) and tabindex is used to specify in which order the keyboard should access it;
  • the tabindex is to make some mouse event (mousedown/click/mouseup) keyboard accessible;
  • there is an event listener (eg. keydown/keypress/keyup) that will only fire when the element is focused, and tabindex makes the initial focusing possible from the keyboard; or
  • it is superfluous.

I expect the first and second types are most common, which is why we use it for link hints. The default link hints mode is so overloaded that it's hard to tell whether the third should be part of its remit. I suspect we shouldn't stop supporting the 2nd and 3rd cases just to stop the 4th from getting in the way.

So we need a rule to pick one. For example, if one is an ancestor of the other, then pick the ancestor. Or, if one is larger than the other, pick the larger.

I disagree; we can activate both using the mouse, so we should be able to do the same with Vimium. The issue we're actually seeing here is that we're showing a link hint for an element at a location where clicking won't act on that element. See here:

screenshot from 2014-12-15 18 43 21 - edit

The child element receives click events in the selected area, but for the rest of the bar they bubble to the parent. By subtracting the rects of all clickable children, we're left with places that clicking does activate the element's listener. Putting a link hint in one of these places instead stops the overlapping hints and removes the ambiguity; a win-win.

(I've been wondering for some time if we need a file hacks.coffee for sites which don't play nicely. But I couldn't possibly say that out loud.)

We could certainly do with some kind of 'experimental options' in the options page, to allow enabling more controversial fixes like #1167, #1211, etc. either per-URL or globally. I especially feel like #1167 should be available for those who want it — it fixes a lot of annoyances on a lot of sites — but is perhaps too risky to ship and run everywhere by default.

I don't like the idea of site-specific hacks very much. In some cases they might prove the only option (#1295 specifically springs to mind) though. I suggest a hacks folder, where each hack is a content script, enabled on new URLs by adding them to the matches in manifest.json. It nearly sounds too clean and easy.

@smblott-github
Copy link
Collaborator

The issue we're actually seeing here is that we're showing a link hint for an element at a location where clicking won't act on that element

This is good insight.

@dragon788
Copy link

From reading a bunch of comments, it sounds like the current workaround, without even requiring passkeys is to enter Insert mode, which will pass raw keystrokes to the page, which will allow f / i / whatever to pass through and control the underlying page. I find this an acceptable workaround on these sites, since I only hit i once to enter insert mode and then everything else is "Google-y".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
@dragon788 @maximbaz @mrmr1993 @smblott-github and others