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

Update selectors for querying events from mozilla #195

Merged
merged 3 commits into from
Apr 22, 2018
Merged

Update selectors for querying events from mozilla #195

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1523567
Update selectors for querying events from mozilla
gernest Apr 15, 2018
5f529e0
Update nameMap for new events
gernest Apr 15, 2018
4382b3e
Update nameMap value for appinstalled to ApplicationInstalled
gernest Apr 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 37 additions & 15 deletions event/event.gen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ func AnimationStart(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "animationstart", Listener: listener}
}

// AppInstalled is an event fired when a web application is successfully
// installed as a progressive web app.
//
// https://developer.mozilla.org/docs/Web/Events/appinstalled
func AppInstalled(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "appinstalled", Listener: listener}
}

// AudioEnd is an event fired when the user agent has finished capturing audio
// for speech recognition.
//
Expand All @@ -53,14 +61,6 @@ func AudioEnd(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "audioend", Listener: listener}
}

// AudioProcess is an event fired when the input buffer of a
// ScriptProcessorNode is ready to be processed.
//
// https://developer.mozilla.org/docs/Web/Events/audioprocess
func AudioProcess(listener func(*vecty.Event)) *vecty.EventListener {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this has gotten removed by the generator even though it is still a valid event (not deprecated etc). We'll need to figure out / fix why this got removed before we can merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slimsag apologies for the late feedback, I was in a hospital , I feel better now. I t seems the cause is the .icon-thumbs-down-alt class see https://github.com/gopherjs/vecty/pull/195/files#diff-7f46ba42e4ad2574c9cbe6836be77d61L140 I don't know why the logic is there. Maybe the docs added that icon for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry to hear you were not feeling well! Hope you're feeling better.

I just had a chance to look at this, and it looks like the problem is just more inconsistency on the MDN -_-

image

The audioprocess event is not marked as deprecated, but the data type it events AudioProcessingEvent is deprecated. Obviously that is silly because that means both are deprecated, so we're doing the right thing here by removing this (since we don't keep deprecated items in here). Just another reason to solve #136 soon.

return &vecty.EventListener{Name: "audioprocess", Listener: listener}
}

// AudioStart is an event fired when the user agent has started to capture
// audio for speech recognition.
//
Expand Down Expand Up @@ -132,9 +132,8 @@ func CanPlay(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "canplay", Listener: listener}
}

// CanPlayThrough is an event fired when the user agent can play the media, and
// estimates that enough data has been loaded to play the media up to its end
// without having to stop for further buffering of content.
// CanPlayThrough is an event fired when the user agent can play the media up
// to its end without having to stop for further buffering of content.
//
// https://developer.mozilla.org/docs/Web/Events/canplaythrough
func CanPlayThrough(listener func(*vecty.Event)) *vecty.EventListener {
Expand Down Expand Up @@ -188,10 +187,9 @@ func Close(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "close", Listener: listener}
}

// Complete is an event fired when the rendering of an OfflineAudioContext is
// terminated.
// Complete is an event fired when a transaction successfully completed.
//
// https://developer.mozilla.org/docs/Web/Events/complete
// https://developer.mozilla.org/docs/Web/Reference/Events/complete_indexedDB
func Complete(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "complete", Listener: listener}
}
Expand Down Expand Up @@ -253,6 +251,14 @@ func DOMContentLoaded(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "DOMContentLoaded", Listener: listener}
}

// DeviceChange is an event fired when a media device such as a camera,
// microphone, or speaker is connected or removed from the system.
//
// https://developer.mozilla.org/docs/Web/Events/devicechange
func DeviceChange(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "devicechange", Listener: listener}
}

// DeviceLight is an event fired when fresh data is available from a light
// sensor.
//
Expand Down Expand Up @@ -376,7 +382,7 @@ func DurationChange(listener func(*vecty.Event)) *vecty.EventListener {

// Emptied is an event fired when the media has become empty; for example, this
// event is sent if the media has already been loaded (or partially loaded),
// and the load() method is called to reload it.
// and the load() method is called to reload it.
Copy link
Member

Choose a reason for hiding this comment

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

We should figure out what changed here. This may be the MDN using another weird space unicode character, and if so, we should swap it out in the generator with a plain ASCII space character.

Copy link
Member

Choose a reason for hiding this comment

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

Probably copying https://github.com/gernest/vecty/blob/5f529e0c4616b201de3fc21c4c8873c92862bf89/elem/generate.go#L158-L160 into this generator would do the trick.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should figure out what changed here.

I already checked. MDN changed a non-breaking whitespace (U+00A0) for a normal whitespace (U+0020).

image

Copying desc = strings.Replace(desc, "\u00a0", " ", -1) might still be desirable if you really want to eliminate those non-breaking whitespaces, in case they come up in the future or elsewhere.

However, I think having non-breaking whitespace in documentation isn't a big deal. I don't see a reason to actively avoid them.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. This change LGTM as-is then.

Unrelated to this change, but I do want to avoid non-breaking whitespace in documentation because it easily leads to changes like this where there is uncertainty about why a line has changed.

//
// https://developer.mozilla.org/docs/Web/Events/emptied
func Emptied(listener func(*vecty.Event)) *vecty.EventListener {
Expand Down Expand Up @@ -594,6 +600,14 @@ func Message(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "message", Listener: listener}
}

// MessageError is an event fired when a message error is raised when a message
// is received by an object.
//
// https://developer.mozilla.org/docs/Web/Events/messageerror
func MessageError(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "messageerror", Listener: listener}
}

// MouseDown is an event fired when a pointing device button (usually a mouse)
// is pressed on an element.
//
Expand Down Expand Up @@ -1029,6 +1043,14 @@ func Show(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "show", Listener: listener}
}

// SlotChange is an event fired when the node contents of a HTMLSlotElement
// (<slot>) have changed.
//
// https://developer.mozilla.org/docs/Web/Events/slotchange
func SlotChange(listener func(*vecty.Event)) *vecty.EventListener {
return &vecty.EventListener{Name: "slotchange", Listener: listener}
}

// SoundEnd is an event fired when any sound — recognisable speech or not —
// has stopped being detected.
//
Expand Down
6 changes: 5 additions & 1 deletion event/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ func main() {
"animationend": "AnimationEnd",
"animationiteration": "AnimationIteration",
"animationstart": "AnimationStart",
"appinstalled": "AppInstalled",
Copy link
Contributor

@dmitshur dmitshur Apr 15, 2018 •
edited

Choose a reason for hiding this comment

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

I'm not sure if "App" should be considered a full word, or if it's short for "Application". If the former, this is good. If the latter, this should be mapped to "ApplicationInstalled", similar to how "dblclick" is mapped to "DoubleClick" below (rather than to "DblClick").

Given that the event is described as (emphasis mine):

The appinstalled event of the Web Manifest API is fired when the browser has successfully installed a page as an application.

I'm leaning slightly towards ApplicationInstalled.

I'll defer to @slimsag on this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, ApplicationInstalled is preferred here.

"audioprocess": "AudioProcess",
"audioend": "AudioEnd",
"audiostart": "AudioStart",
Expand All @@ -45,6 +46,7 @@ func main() {
"compositionupdate": "CompositionUpdate",
"contextmenu": "ContextMenu",
"dblclick": "DoubleClick",
"devicechange": "DeviceChange",
"devicelight": "DeviceLight",
"devicemotion": "DeviceMotion",
"deviceorientation": "DeviceOrientation",
Expand Down Expand Up @@ -74,6 +76,7 @@ func main() {
"loadend": "LoadEnd",
"loadstart": "LoadStart",
"lostpointercapture": "LostPointerCapture",
"messageerror": "MessageError",
"mousedown": "MouseDown",
"mouseenter": "MouseEnter",
"mouseleave": "MouseLeave",
Expand Down Expand Up @@ -104,6 +107,7 @@ func main() {
"resourcetimingbufferfull": "ResourceTimingBufferFull",
"selectstart": "SelectStart",
"selectionchange": "SelectionChange",
"slotchange": "SlotChange",
"soundend": "SoundEnd",
"soundstart": "SoundStart",
"speechend": "SpeechEnd",
Expand Down Expand Up @@ -135,7 +139,7 @@ func main() {

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect it's a good idea to update nameMap above so the generated names have proper casing:

https://github.com/gopherjs/vecty/blob/19a11ec3acc3ae6c2e095b1ace2b7134c5f28c10/event/generate.go#L22-L129

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please expand on what you mean by proper casing? The goal of this PR is just to fix the selector to properly query the page. I believe other changes can be more meaningful in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. This PR currently adds some new functions to event package, like:

  • Devicechange
  • Messageerror
  • Slotchange
  • etc.

According to Go style conventions (https://golang.org/s/style#mixed-caps and https://golang.org/s/style#initialisms), identifiers should use MixedCaps casing. So the new functions should be named:

  • DeviceChange
  • MessageError
  • SlotChange
  • etc.

nameMap provides a mapping from HTML attribute names to proper Go style names. By adding new entries there, you can ensure the functions that are added as part of this PR follow Go style.

If your goal is not to add any new functions to event package as part of this PR, you'll need to make changes. That's not currently the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I will add the mapping, it will be weird just submitting the changes in the generator file without the updated generated file.

events := make(map[string]*Event)

doc.Find(".standard-table").Eq(0).Find("tr").Each(func(i int, s *goquery.Selection) {
doc.Find("#Standard_events+p+.standard-table").Eq(0).Find("tr").Each(func(i int, s *goquery.Selection) {
cols := s.Find("td")
if cols.Length() == 0 || cols.Find(".icon-thumbs-down-alt").Length() != 0 {
return
Expand Down