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.

Sign up for GitHub

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

Jump to bottom

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

annevk
annevk reviewed Jan 8, 2016
View reviewed changes
source
<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.

annevk
annevk reviewed Jan 11, 2016
View reviewed changes
source
<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.

@domenic domenic added the removal/deprecation Removing or deprecating a feature label Jan 11, 2016
@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:

annevk
annevk reviewed Jan 15, 2016
View reviewed changes
source

<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.

annevk
annevk reviewed Jan 18, 2016
View reviewed changes
source
<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?

annevk
annevk reviewed Jan 21, 2016
View reviewed changes
source
<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
@annevk
Regression: define source browsing context for iframe and frame
d57c4c6 
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.
@annevk annevk mentioned this pull request May 3, 2016
Merged
domenic pushed a commit that referenced this pull request May 3, 2016
@annevk @domenic
Regression fix: define source browsing context for iframe and frame
a714902 
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
Merged
@waldyrious waldyrious mentioned this pull request Jul 23, 2016
Open
zcorpan referenced this pull request Sep 9, 2016
@Ritsyy @annevk
Remove <iframe seamless>
1490eba 
This reverts edits made in the following set of commits:

dfa3f22
8edfe6d
9e4f095
b0b5d7b
b3bf8ad
e5a041e
cc33290
c4d6396
f6490f1
77e24cd
0eab1dd
1782006
2898fa9
3a6cfaf
5ae9881
f1c0cad
748113c
4814a86
9dd420c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@annevk annevk

Labels
removal/deprecation Removing or deprecating a feature
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ritsyy @annevk @foolip @domenic