-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add data for history.pushState/replaceState's title argument #5101
Add data for history.pushState/replaceState's title argument #5101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, thanks for your PR! It looks good to me, only a couple of things. We can fill in the rest of the data as well. Since Opera, Opera Android, and WebView Android are based upon Chrome, we can set them to false
as well. We can also probably set Safari iOS to true
too, though of course that needs a little more testing!
I think what I have contributed is better than not having it at all and I have no intention of testing on other browsers. Anyone is welcome to submit their own commit that adds to this.
I can't say with confidence that those engines didn't modify this behaviour so that's why I didn't include them.
Did you confirm that Safari iOS actually shows the title in their UI? That wasn't clear to me from the discussion in whatwg/html#2174 so I didn't include it. As I said above, someone is free to test it an contribute that info if they have it. |
I think it's highly unlikely that the Chrome-based browsers would add functionality -- in fact, I have not once seen one of them with an API feature that wasn't already in Chrome itself, only the other way around. As such, I'm fully confident they would also be As for Safari iOS, that one's just an assumption, as I haven't actually tested it out just yet. Safari iOS could be left |
648e836
to
e6be1d2
Compare
"description": "Whether the <code>title</code> argument is used", | ||
"support": { | ||
"chrome": { | ||
"version_added": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chrome, Chrome Android, and WebView Android support this argument.
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/history.idl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you read whatwg/html#2174? It explains that it's a no-op and therefore isn't "supported". I'm documenting this to avoid the issues described in whatwg/html#2174 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diving through the code reveals that the title
argument, even though it's passed in, is not actually used in the underlying function call.
Hey @space1012 — while reviews are nice, there are many other places where we need assistance. Please refer to https://github.com/mdn/browser-compat-data/blob/master/docs/contributing.md to find out how you can better help this project! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good now, thanks for the PR! I'd say we're ready to merge. 😉
Speaking of, @mnoorenberghe, I had noticed you made a force push to your branch. Just wanted to let you know that, in case it's to simplify the commits, we actually strictly squash merge in this repository!
Most browsers don't do anything with the
title
argument so developers get confused by it and that's why I think it should be documented as a sub-feature. See whatwg/html#2174A checklist to help your pull request get merged faster: