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 the dropzone global attribute. #2402

Merged
merged 2 commits into from Mar 6, 2017

Conversation

3 participants
@pwnall
Contributor

pwnall commented Feb 25, 2017

dropzone failed to get traction among browser implementers. Having it in the specification is confusing to Web developers who may attempt to use it, only to discover that it is not supported.

Safari and Chrome implemented a prefixed version, webkitdropzone. The prefixed version is going away from Chrome [1]. Other browser vendors have no objections to the attribute getting removed from the spec [2].

[1] https://www.chromestatus.com/feature/5718005866561536
[2] #2331

This fixes #2331.

@domenic

This comment has been minimized.

Member

domenic commented Feb 25, 2017

Thanks for doing this!

It looks like there are some build errors; would you mind fixing so we can review more easily?

@pwnall

This comment has been minimized.

Contributor

pwnall commented Feb 25, 2017

@domenic On it! I'm still trying to figure out what the errors mean, but I will definitely not give up until I get a green build.

@pwnall

This comment has been minimized.

Contributor

pwnall commented Feb 25, 2017

@domenic Can you please take another look?

I was looking at the line of the first error, until I realized that the 2nd line number is smaller than the first. 😅

@domenic

This comment has been minimized.

Member

domenic commented Feb 25, 2017

Build is passing now, so I'll be able to compile locally and do a check :). It's the weekend at this point though so I'll take a look Monday!

@pwnall

This comment has been minimized.

Contributor

pwnall commented Feb 25, 2017

Thank you very much!

@domenic

A few minor things to fix, but overall looking great!

source Outdated
whether or not the drop target is potentially willing to accept the drop.</li>
<li>The <code data-x="event-dnd-dragover">dragover</code> event handler must specify what
feedback will be shown to the user.</li>

This comment has been minimized.

@domenic

domenic Feb 27, 2017

Member

Since this is a non-normative section, we should not be using "must" (or similar words). Not sure on the best rephrasing... maybe "can"?

This comment has been minimized.

@pwnall

pwnall Feb 28, 2017

Contributor

I tried a draft that simply removed the "must". WDYT?

This comment has been minimized.

@domenic

domenic Feb 28, 2017

Member

Looks good!

source Outdated
give (e.g. "<code data-x="">move</code>" to indicate that the data will be moved).</p>
<ol>
<li>The <code data-x="event-dnd-dragenter">dragenter</code> event handler must report

This comment has been minimized.

@domenic

domenic Feb 27, 2017

Member

It might be helpful to add something like "by canceling the event"

This comment has been minimized.

@pwnall

pwnall Feb 28, 2017

Contributor

Done. Thank you for the suggestion! Thinking back to the first time I coded up drag-and-drop, more clarity here is super-helpful.

source Outdated
}
function dragOverHandler(event) {
event.dataTransfer.dropEffect = 'move';
}
function dropHandler(event) {

This comment has been minimized.

@domenic

domenic Feb 27, 2017

Member

The example below this one also needs updating, to stay in sync.

This comment has been minimized.

@pwnall

pwnall Feb 28, 2017

Contributor

I tried out the full example, and (assuming I got it right) the code fragment below doesn't need upgrading.

This comment has been minimized.

@domenic

domenic Feb 28, 2017

Member

Well, the issue is that the code fragment below is supposed to be a modification of the above one (which you modified) to simply add dragend handling. So since you added dragenter/dragover handling to the first fragment, the second fragment should get updated to match.

This comment has been minimized.

@pwnall

pwnall Feb 28, 2017

Contributor

GitHub seems to fail at expanding the diff (perhaps because source is a large file), so you'll have to use a local checkout to look at the lines I'm referencing here. Sorry about that!

IIUC, you're referring to lines 74331-74345. They're an update of lines 74244-74261, which I haven't changed. Can you please explain me what I got wrong? Also, thank you for bearing with me!

This comment has been minimized.

@domenic

domenic Mar 2, 2017

Member

No problem!

The issue is that lines 74331-74345 are meant to be exactly the same example as lines 74289-74322, but with the addition of also adding a dragend handler. That is, consider the sentence between the two examples, annotated:

For our example here [i.e., the example on lines 74289-74322], that means updating the original markup [shown in those lines] to handle that [dragend] event, [as shown below by modifying the original markup but also adding some dragend-specific stuff].

This it's important that lines 74331-74345 match lines 74289-74322, except:

  • They should add dragend handling
  • They can use "... as before ..." to avoid repetition. (I might just do // dragEnterHandler, dragOverHandler, and dropHandler as before instead of the current function dragStartHandler(event) { // <em>...as before...</em> })

This comment has been minimized.

@domenic

domenic Mar 2, 2017

Member

Clarified offline: I was confused. As @pwnall says, lines 74331-74345 are an update of lines 74244-74261, not an update of lines 74289-74322 like I thought.

@pwnall

This comment has been minimized.

Contributor

pwnall commented Feb 28, 2017

@domenic Thank you very much for your thoughtful feedback! PTAL?

@domenic

domenic approved these changes Mar 2, 2017

@pwnall

This comment has been minimized.

Contributor

pwnall commented Mar 2, 2017

@domenic Thank you very much for helping me with my first contribution!

@domenic

This comment has been minimized.

Member

domenic commented Mar 2, 2017

This LGTM! I forgot to ask though, we are trying to get web platform tests before merging any changes. I'm happy to help with that if you want, just let me know. Mostly it will be a matter of:

@domenic domenic added the needs tests label Mar 2, 2017

@pwnall

This comment has been minimized.

Contributor

pwnall commented Mar 2, 2017

@domenic I started working on WPT.

I didn't remove everything with dropzone because I suspect that some other APIs (e.g. DataTransfer.dropEffect) might only be covered in tests that have dropzone. This was definitely the case in Blink. Should I ping this PR when I've removed everything?

@domenic

This comment has been minimized.

Member

domenic commented Mar 2, 2017

Hmm, interesting. How long do you think that would take? I don't think we should hold up merging this much longer. So I think it would be fine to merge this with even just a small test addition to https://github.com/w3c/web-platform-tests/blob/master/html/dom/elements/elements-in-the-dom/historical.html and an open issue on the web-platform-tests repository tracking the removal of the rest.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Mar 3, 2017

dropzone has existed in the spec since 2010, so it seems reasonable to add it to the Obsolete section (with id="the-dropzone-attribute").

Otherwise LGTM.

dumganhar pushed a commit to dumganhar/chromium that referenced this pull request Mar 3, 2017

Remove webkitdropzone.
webkitdropzone is a prefixed version of the dropzone HTML global
attribute. dropzone failed to gain traction among browser vendors, and
only Blink and WebKit implement prefixed versions. Therefore, dropzone
is being removed from the HTML specification in
whatwg/html#2402

The following LayoutTests, which depended on webkitdropzone, have been
rewritten to use event listeners. These tests cover other drag-and-drop
functionality that we still want covered.

* fast/events/dropzone-001 -> fast/dnd/dropEffect-for-effectAllowed
* fast/events/dropzone-002 -> fast/dnd/dropEffect-for-image
* fast/events/dropzone-003 -> fast/dnd/dropEffect-for-link
* fast/events/dropzone-004 -> fast/dnd/dropEffect-for-file

The followng LayoutTests have been removed, because they do not cover
additional functionality that we still need.

* fast/events/dropzone-005 -> only covers dropzone-specific code
* fast/dnd/file-drop-on-webkitdropzone-element.html -> redundant with
  fast/dnd/file-drag-drop-on-page.html
* http/tests/dnd/file-drop-on-webkitdropzone-element.html -> redundant with
  fast/dnd/file-drag-drop-on-page.html

BUG=688943

Review-Url: https://codereview.chromium.org/2720463002
Cr-Commit-Position: refs/heads/master@{#454488}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Mar 6, 2017

Remove webkitdropzone.
webkitdropzone is a prefixed version of the dropzone HTML global
attribute. dropzone failed to gain traction among browser vendors, and
only Blink and WebKit implement prefixed versions. Therefore, dropzone
is being removed from the HTML specification in
whatwg/html#2402

The following LayoutTests, which depended on webkitdropzone, have been
rewritten to use event listeners. These tests cover other drag-and-drop
functionality that we still want covered.

* fast/events/dropzone-001 -> fast/dnd/dropEffect-for-effectAllowed
* fast/events/dropzone-002 -> fast/dnd/dropEffect-for-image
* fast/events/dropzone-003 -> fast/dnd/dropEffect-for-link
* fast/events/dropzone-004 -> fast/dnd/dropEffect-for-file

The followng LayoutTests have been removed, because they do not cover
additional functionality that we still need.

* fast/events/dropzone-005 -> only covers dropzone-specific code
* fast/dnd/file-drop-on-webkitdropzone-element.html -> redundant with
  fast/dnd/file-drag-drop-on-page.html
* http/tests/dnd/file-drop-on-webkitdropzone-element.html -> redundant with
  fast/dnd/file-drag-drop-on-page.html

BUG=688943

Review-Url: https://codereview.chromium.org/2720463002
Cr-Commit-Position: refs/heads/master@{#454488}
(cherry picked from commit 798a0d0)

Review-Url: https://codereview.chromium.org/2729353002 .
Cr-Commit-Position: refs/branch-heads/3029@{#9}
Cr-Branched-From: 939b32e-refs/heads/master@{#454471}
Remove the dropzone global attribute.
dropzone failed to get traction among browser implementers. Having it in
the specification is confusing to Web developers who may attempt to use
it, only to discover that it is not supported.

Safari and Chrome implemented a prefixed version, webkitdropzone. The
prefixed version is going away from Chrome [1]. Other browser vendors
have no objections to the attribute getting removed from the spec [2].

[1] https://www.chromestatus.com/feature/5718005866561536
[2] #2331
@pwnall

This comment has been minimized.

Contributor

pwnall commented Mar 6, 2017

@zcorpan I added dropzone to the Obsolete section. Can you please take a look and check that I used appropriate text?

pwnall added a commit to pwnall/web-platform-tests that referenced this pull request Mar 6, 2017

Add dropzone to the list of obsoleted HTML elements.
dropzone is getting removed from the HTML standard in
whatwg/html#2402
@pwnall

This comment has been minimized.

Contributor

pwnall commented Mar 6, 2017

@domenic Thank you very much for your pragmatic advice!

I submitted web-platform-tests/wpt#5052 for adding dropzone to the historical checks, and filed web-platform-tests/wpt#5053

Tweak position of the obsolete entry
Let's leave the massive "use CSS instead of all these presentational attributes" section at the bottom.
@domenic

This comment has been minimized.

Member

domenic commented Mar 6, 2017

It all looks good to me! I pushed a small tweak to move it before all the "use CSS instead" items. I'll merge after the CI completes!

domenic added a commit to web-platform-tests/wpt that referenced this pull request Mar 6, 2017

Add dropzone to the list of obsoleted HTML elements
dropzone is getting removed from the HTML standard in
whatwg/html#2402.

@domenic domenic merged commit 3044a7e into whatwg:master Mar 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pwnall

This comment has been minimized.

Contributor

pwnall commented Mar 6, 2017

Thank you very much for the feedback and for all the help & patience!

@pwnall pwnall deleted the pwnall:rm-dropzone branch May 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment