search

guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.5k stars 245 forks source link

Focus handler breaks when Scribe's content has been set to '' #368

Closed rf- closed 9 years ago

rf- commented 9 years ago

The handler for the focus event breaks when scribe.el.firstChild is null, which happens if the editor has been emptied using scribe.setContent(''). The specific error is Uncaught TypeError: Failed to execute 'createTreeWalker' on 'Document': parameter 1 is not of type 'Node'..

I'm prepared to accept "don't do that" as an answer, but it'd be pretty inconvenient. It naively seems like var focusElement = getFirstDeepestChild(scribe.el) might fix it?

rrees commented 9 years ago

I see what you're saying but if there isn't a child you don't want to do the walk right? So in getFirstDeepestChild you shouldn't do the walk but maybe return undefined and then in the focus function focus the scribe root element if getFirstDeepestChild didn't return an element.

rf- commented 9 years ago

Yeah, that's reasonable. getFirstDeepestChild is already capable of returning the same element you pass to it though (like if scribe.el.firstChild exists but has no children), so it seems simpler to just start the descent one level higher instead of adding a special case.

rrees commented 9 years ago

Okay, I'll look at making the change

gon commented 9 years ago

@rrees I'm having the same issue, any plans to merge in a fix for it?

rrees commented 9 years ago

Yes. I know you're not going to believe me but I thought I might tackle it tomorrow. Nag me if I don't.

gon commented 9 years ago

Hat bow.

rrees commented 9 years ago

@gon @rf- Can you test the fix in 1.3.9?

rrees commented 9 years ago

Assuming this is good now.

rf- commented 9 years ago

Sorry, this has been near the top of my todo list for like a month. :( I'm actually about to resume work on the part of our app that uses Scribe, so I'll move us back onto upstream and let you know if there are any issues.

lencioni commented 9 years ago

I just tried upgrading the app that @rf- has been working with to 1.4.15, and things seem to be working fine. Thanks for fixing this!