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

Bring the preview tab to the front when clicking the preview button. #9015

Merged
merged 1 commit into from
Aug 15, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Aug 15, 2018

Testing instructions

  • Try clicking the preview button, the preview tab should be brought to the front
  • Same if you click multiple times
  • Same if you click after making edits while the preview tab is open

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Aug 15, 2018
@youknowriad youknowriad added this to the 3.6 milestone Aug 15, 2018
@youknowriad youknowriad self-assigned this Aug 15, 2018
@youknowriad youknowriad requested review from jasmussen, aduth and a team August 15, 2018 12:58
@aduth
Copy link
Member

aduth commented Aug 15, 2018

Duplicates #8446

@youknowriad
Copy link
Contributor Author

oups :)

@aduth
Copy link
Member

aduth commented Aug 15, 2018

We also need to refocus the window in all cases.

Can you elaborate on this? Is this intended to bring the tab to the front? I was under the impression this wouldn't be possible, but if focus works, I'm all for it.

@youknowriad
Copy link
Contributor Author

Can you elaborate on this? Is this intended to bring the tab to the front?

Yes, exactly. I assume we need to test in several browsers, but yeah it works

@youknowriad youknowriad deleted the fix/preview-button branch August 15, 2018 13:23
@aduth
Copy link
Member

aduth commented Aug 15, 2018

Yes, exactly. I assume we need to test in several browsers, but yeah it works

I mean, even if it doesn't work, it could be considered an enhancement for those which do. I assumed it wasn't possible. The Classic Editor, for example, often loads to an existing tab but doesn't shift focus when saving.

Maybe we could reopen this one and repurpose it toward just that change?

@youknowriad youknowriad restored the fix/preview-button branch August 15, 2018 13:29
@youknowriad youknowriad reopened this Aug 15, 2018
@youknowriad youknowriad changed the title Fix simultaneous click on the preview button Bring the preview tab to the front when clicking the preview button. Aug 15, 2018
@youknowriad
Copy link
Contributor Author

So this seems to work only on Chrome for me but doesn't break other browsers, so yeah. Progressive Enhancement.

@tofumatt
Copy link
Member

If this works only in Chrome, as you say, we should leave a comment explaining that, because otherwise this might come across as quite the bug.

Would be a good candidate for an e2e test too, especially as the behaviour is only present in Chrome ^_^

@jasmussen
Copy link
Contributor

I love progressive enhancement.

But is there any way to know if this is, you know, a web standard? Is this Chrome being early in implementing a feature, or is it Chrome being Chrome? If it's a feature that should eventually trickle out to other browsers, ship it. But otherwise, probably good to generally try and avoid platform lockin.

@youknowriad youknowriad force-pushed the fix/preview-button branch 2 times, most recently from 73b131a to a9a1591 Compare August 15, 2018 14:51
@youknowriad
Copy link
Contributor Author

It looks like this is standard but it's configurable by the user in the browser settings. I guess browser have different default values for these options https://developer.mozilla.org/en-US/docs/Web/API/Window/focus

@aduth
Copy link
Member

aduth commented Aug 15, 2018

The related specification is an interesting read as well:

The focus() method on the Window object, when invoked, must run the focusing steps with the Window object's browsing context. Additionally, if this browsing context is a top-level browsing context, user agents are encouraged to trigger some sort of notification to indicate to the user that the page is attempting to gain focus.

https://html.spec.whatwg.org/multipage/interaction.html#dom-window-focus

Effectively, this is one of those things where it's left to the browser to decide how to implement, but the intent of the change is to indicate that the tab is intended to be brought to the forefront (whether this actually occurs or some other indicator of intent like flashing, etc).

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

With #8446 being merged, I'm not sure whether to expect there's still changes shown for adding the || this.previewWindow.closed, but otherwise this looks good 👍

@youknowriad youknowriad merged commit d9ef0ed into master Aug 15, 2018
@youknowriad youknowriad deleted the fix/preview-button branch August 15, 2018 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants