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

Remove <iframe seamless> #484

Closed
wants to merge 1 commit into from
Closed

Remove <iframe seamless> #484

wants to merge 1 commit into from

Conversation

Ritsyy
Copy link
Contributor

@Ritsyy Ritsyy commented Jan 7, 2016

Fix #331

As per http://caniuse.com/#search=seamless , it doesn't seem to be supported.
Hence it is a good candidate for removal.

Commits related to <iframe seamless>:
dfa3f22b5
8edfe6da5
9e4f0956b
b0b5d7b16
b3bf8adba
e5a041ecd
cc33290d6
c4d639680

<p class="note">The <code data-x="attr-contenteditable">contenteditable</code> attribute does not
propagate into <code data-x="attr-iframe-seamless">seamless</code> <code>iframe</code>s.</p>

<hr>
Copy link
Member

Choose a reason for hiding this comment

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

Since this is immediately followed by <hr> <!-- FULLSCREEN -->, we can drop this, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the hr tag right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and perhaps some newlines around it so the amount of spacing between bits of text remains equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, okay.

@annevk
Copy link
Member

annevk commented Jan 8, 2016

This does not yet remove all instances of "explicit self-navigation override". (Since it does remove the definition I would assume it no longer compiles, but I guess it does?)

It also has not removed the rendering section rules or the mentions of the attribute in the indexes.

@Ritsyy
Copy link
Contributor Author

Ritsyy commented Jan 8, 2016

@annevk yes, there is one instance which i have skipped of "explicit self-navigation override", for compiling i didn't got your point.

yes the indexes, right removing them.

<span>browsing context</span> does not have its <span>seamless browsing context flag</span> set, a
default value of 8px is expected to be used for that property instead.</p>

cannot be parsed successfully, then a default value of 8px is expected to be used for that property instead.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Needs wrapping.

@annevk
Copy link
Member

annevk commented Jan 12, 2016

@Ritsyy so I think this looks good but it would be good if you updated the commit message to list the dozen or so commits this commit will revert. The addition of seamless was spread over many commits and it would be good to have the commit message point them all out so we can be sure nothing is forgotten.

@Ritsyy
Copy link
Contributor Author

Ritsyy commented Jan 12, 2016

@annevk yes, i'll add all the commits.

@annevk
Copy link
Member

annevk commented Jan 15, 2016

@Ritsyy hey, I still find more commits. These are all I found:


<p>For each property in the table below, given a <code>body</code> element,
the first attribute that exists <span>maps to the pixel length property</
span> on the <code>body</code> element. If none of the attributes for a
Copy link
Member

Choose a reason for hiding this comment

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

You cannot wrap </span> this way. When running the build script this returns an error.

<p>The IDL attributes <dfn><code data-x="dom-iframe-src">src</code></dfn>, <dfn><code data-x="dom-iframe-srcdoc">srcdoc</code></dfn>, <dfn><code data-x="dom-iframe-name">name</code></dfn>, <dfn><code data-x="dom-iframe-sandbox">sandbox</code></dfn>, and <dfn><code data-x="dom-iframe-seamless">seamless</code></dfn> must <span>reflect</span> the respective
content attributes of the same name.</p>
<p>The IDL attributes <dfn><code data-x="dom-iframe-src">src</code></dfn>, <dfn><code data-x="dom-
iframe-srcdoc">srcdoc</code></dfn>, <dfn><code data-x="dom-iframe-name">name</code></dfn>, and
Copy link
Member

Choose a reason for hiding this comment

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

You cannot wrap inside an attribute value like this. That will cause whitespace to appear there.

@Ritsyy Ritsyy force-pushed the Bug331 branch 4 times, most recently from 61f585e to 42c9af7 Compare January 21, 2016 16:39
@Ritsyy
Copy link
Contributor Author

Ritsyy commented Jan 21, 2016

@annevk i have fixed the errors and wrapping issues, committed the changes. Thanks!

@annevk
Copy link
Member

annevk commented Jan 21, 2016

@Ritsyy did you also get Error: Could not find ID attr-iframe-seamless for annotation that uses URLs:? (This is fine, just want to check if the build process is working for you.)

Could you rebase this on master as well?

<p>The IDL attributes <dfn><code data-x="dom-iframe-src">src</code></dfn>, <dfn><code data-x="dom-iframe-srcdoc">srcdoc</code></dfn>, <dfn><code data-x="dom-iframe-name">name</code></dfn>, <dfn><code data-x="dom-iframe-sandbox">sandbox</code></dfn>, and <dfn><code data-x="dom-iframe-seamless">seamless</code></dfn> must <span>reflect</span> the respective
<p>The IDL attributes <dfn><code data-x="dom-iframe-src">src</code></dfn>,
<dfn><code data-x="dom-iframe-srcdoc">srcdoc</code></dfn>, <dfn><code data-x="dom-iframe-name">name</code></dfn>, and
<dfn><code data-x="dom-iframe-sandbox">sandbox</code></dfn> must <span>reflect</span> the respective
Copy link
Member

Choose a reason for hiding this comment

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

This is still incorrectly wrapped. Remember, no more than 100 columns per line and you can break after e.g., <dfn><code if that makes the current line longer.

  <p>The IDL attributes <dfn><code data-x="dom-iframe-src">src</code></dfn>, <dfn><code
  data-x="dom-iframe-srcdoc">srcdoc</code></dfn>, <dfn><code 
  data-x="dom-iframe-name">name</code></dfn>, and <dfn><code
  data-x="dom-iframe-sandbox">sandbox</code></dfn> must <span>reflect</span> the respective content
  attributes of the same name.</p>

Is what I get when I follow those guidelines.

@Ritsyy Ritsyy force-pushed the Bug331 branch 2 times, most recently from c93e2f8 to f236f62 Compare January 22, 2016 09:32
@annevk
Copy link
Member

annevk commented Jan 22, 2016

Committed as 1490eba.

@annevk annevk closed this Jan 22, 2016
@foolip
Copy link
Member

foolip commented Jan 25, 2016

You forgot to link to this PR in the commit message.

I'm seeing the Warning: Could not find ID attr-iframe-seamless for annotation that uses URLs: http://caniuse.com/#feat=iframe-seamless problem now as well, but I see that's tracked by Fyrd/caniuse#2232

@foolip
Copy link
Member

foolip commented Jan 25, 2016

Or rather, I guess that the commit message should have linked to #331

@annevk
Copy link
Member

annevk commented Jan 25, 2016

Yeah, apologies, I messed up rather badly.

annevk added a commit that referenced this pull request May 3, 2016
1490eba, from PR #484, removed the
source browsing context definition along with "explicit
self-navigation override". This restores the definition of source
browsing context for these elements as it was before that commit.

This fixes #1131, but note that per #1130 further changes are
required here, as browsing contexts are not a good concept to use as
source.
domenic pushed a commit that referenced this pull request May 3, 2016
1490eba, from PR #484, removed the
source browsing context definition while removing the "explicit
self-navigation override". This restores the definition of source
browsing context for these elements as it was before that commit.

This fixes #1131, but note that per #1130 further changes are
required here, as browsing contexts are not a good concept to use as
source.
@domenic domenic mentioned this pull request Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removal/deprecation Removing or deprecating a feature
Development

Successfully merging this pull request may close these issues.

4 participants