Community
Participate
Working Groups
Build Identifier: Currently we can create an instance of TextView and use it immediately (sync operation). However, the TextView _init() method does some operations that are prone to failure: it loads the stylesheet with an XMLHttpRequest, it creates an iframe, it writes to it, and so on. If I create a new TextModel("with some text") that I give to TextView, then I immediately call view.setSelection(a, b)... I get failures. (an exception in _getTopIndex() which is used by _showCaret()) I would expect that there would an async initialization mechanism by default, one that would allow us to give it a callback, so we know when the editor is usable. For the Mozilla codebase integration I assume initialization is async. Reproducible: Always
(In reply to comment #0) > If I create a new TextModel("with some text") that I give to TextView, then I > immediately call view.setSelection(a, b)... I get failures. (an exception in > _getTopIndex() which is used by _showCaret()) That is a different bug (we basically missing a call to updatePage() at the end of init()). I believe we can fix this without having an async initialization mechanism. > I would expect that there would an async initialization mechanism by default, > one that would allow us to give it a callback, so we know when the editor is > usable. > For the Mozilla codebase integration I assume initialization is async. > Reproducible: Always But the fact that the textView loads synchronously is causing problems to you ? I mean, in the editor code you wait for all load events and create the textview only when everything is ready. Then you can send callbacks to other components that use the textview. I thought that having the a synchronous initialization was good for client that need async or sync use of the textview. That said, we might have to add async initialization mechanism to fix https://bugs.eclipse.org/bugs/show_bug.cgi?id=337160#c4
(In reply to comment #1) > > If I create a new TextModel("with some text") that I give to TextView, then I > > immediately call view.setSelection(a, b)... I get failures. (an exception in > > _getTopIndex() which is used by _showCaret()) > That is a different bug (we basically missing a call to updatePage() at the end > of init()). I believe we can fix this without having an async initialization > mechanism. See Bug 349714
(In reply to comment #1) > (In reply to comment #0) > > If I create a new TextModel("with some text") that I give to TextView, then I > > immediately call view.setSelection(a, b)... I get failures. (an exception in > > _getTopIndex() which is used by _showCaret()) > > That is a different bug (we basically missing a call to updatePage() at the end > of init()). I believe we can fix this without having an async initialization > mechanism. Ah, I didn't investigate the issue so much. > > I would expect that there would an async initialization mechanism by default, > > one that would allow us to give it a callback, so we know when the editor is > > usable. > > For the Mozilla codebase integration I assume initialization is async. > > Reproducible: Always > > But the fact that the textView loads synchronously is causing problems to you ? > > I mean, in the editor code you wait for all load events and create the textview > only when everything is ready. Then you can send callbacks to other components > that use the textview. > > I thought that having the a synchronous initialization was good for client that > need async or sync use of the textview. > > That said, we might have to add async initialization mechanism to fix > https://bugs.eclipse.org/bugs/show_bug.cgi?id=337160#c4 If the fix you have in mind works, then, sure, the sync loading of TextView does not cause problems for me, at the moment. Still, the sync loading is prone to errors and greatly limits the initialization flexibility of the TextView, in the future, for the Orion team. In Mozilla's SourceEditor component that abstracts Orion I chose to have a specific call to init() the editor - initialization does not happen on SourceEditor object construction. Additionally, init() takes a callback to inform the user when initialization is complete. That's how far async stuff I would see in Orion's TextView implementation as well. Obviously, if you can keep stuff in sync, there's no reason to go for async, but take into consideration that, for example, the XMLHttpRequest blocks the UI, the browser, while it fetches the stylesheet. If there's network congestion or simply there's a remote network problem, the user is going to wait with a frozen UI, for quite some time (due to network request timeouts). If you are going to change the initialization, I have the following suggestions: - Put the stylesheet in the iframe <head>, as a <link>, not dumped into a <style> element. Wait for the iframe load event, and so on. - Avoid the use of document.write(). Switch to a data URI. You can have a minimal data URI that just sets the DTD and the style: "data:text/html,<!DOCTYPE html><link rel=stylesheet href=...><p>hello world!". Done. When the iframe loads, do the rest. (You do not need <html>/<head>/<body> tags in the data URI.) Obviously, these are just some ideas. Implementing them means something like "Your Mileage May Vary".
We are still having issues crop out because the Orion editor initialization is (not quite) sync. Is there a fix for this bug in the roadmap for Orion's next milestones?
We added asynchronous loading to the latest code (see bug#337160). Now, it is possible to create the editor when it is not connected to the DOM or hidden because of display:none. Initialization is still synchronous when possible. The frame HTML is still initialized using document.write() and the style sheets are still loaded with XMLHttpRequest. We investigated data URIs, but found the following problems: - IE does not support setting IFRAME.src with data URIs - Webkit does not work either. It outputs some warning in the console saying that it is unsafe to access the frame URI. It seems it is not possible to get the body to the frame document because of security reasons (it is not in the same domain?). - In Firefox, we were not able to make the <link> elements load its content. It works if the style sheet is loaded synchronously with XMLHttpRequest and embedded in the URI, but that defeats the work. We have not provided a callback to determine when the editor is initialized. It is not clear this callback is necessary, since many APIs work while the editor is not initialized . See bug#337160 for a list. If we decide to add the callback, we will need two: load and unload; since the editor could be disconnected (hidden) after creation.
I'm now running into problems with our use of a synch XHR to write out and load the CSS file. The CSS file was using an absolute local url (e.g. a local url prefixed with '/') but we've now switched to using relative urls for everything in the client. CSS rules are written such that any url(...) type style values are calculated relative to the CSS file location. By inlining it you make those style rules relative to the HTML file. If we continue writing out CSS files in this fashion then we will at least have to rewrite the url(...) stuff to be relative to the CSS file location. -- more info here -- http://requirejs.org/docs/faq-advanced.html#css http://bugs.dojotoolkit.org/ticket/5402 https://bugzilla.mozilla.org/show_bug.cgi?id=185236 -- This just in!! -- noticed https://bugzilla.mozilla.org/show_bug.cgi?id=185236 was fixed a couple of weeks ago so maybe we can do something smart here and wait for a load event.
For completeness -- still doesn't work in WebKit -- https://bugs.webkit.org/show_bug.cgi?id=38995 Here's a quick test link from James Burke -- http://www.tagneto.org/blogcode/185236/loadtest.html
I investigated data URIs further and I was able to make it work on Firefox (and Opera). I added a <base> element to the data URI <head> pointing to the same location of the parent document. This ensured that the links could be found. html.push("<head>"); var baseURI = window.location; html.push("<base href='" + baseURI + "'/>"); Chrome fails because of this bug: https://bugs.webkit.org/show_bug.cgi?id=17352
Thanks for your work on the async initialization of the editor. I looked into the code and I see a DOMAttrModified mutation event listener is used for the parent document. This causes a pretty negative performance hit for all subsequent DOM operations in Gecko (and most-likely other browsers as well), in that parent document. Has this problem been considered?
Yes, we considered this performance hit. We did not really measure what was the impact. Do you have any numbers? Note that DOMAttrModified is only really necessary for Firefox because in IMO Firefox has a bug where getComputedStyle() returns null for elements inside an IFRAME which is hidden because some parent has display:none. This works fine in other browsers (WebKit, IE >9 and Opera). I will attach sample code that shows this. I believe the alternatives for DOMAttrModified in this case are: 1) use a timer that keeps pooling when the display style changes. It does not seem any cleaner, but it may performance better. 2) Stop using IFRAME all together. This is probably the best option in the long run, but it is not a trial task and I am not sure it is possible given that we are using fixed divs for scrolling. 3) Do not handle this at all and let the application code only create the text view when the parent is made visible (display:block). This just shift the burden to the app code. 4) Fix mozilla's bug https://bugzilla.mozilla.org/show_bug.cgi?id=548397 . Can you think of any other solution?
Created attachment 204958 [details] sample for comment#10
(In reply to comment #10) > Yes, we considered this performance hit. We did not really measure what was the > impact. Do you have any numbers? I haven't had the time to do any such testing to provide numbers. I just know from the Gecko developers that we must avoid DOM mutation event listeners as much as possible. Also, Firefox UI code is not allowed to use them. > I believe the alternatives for DOMAttrModified in this case are: > > 1) use a timer that keeps pooling when the display style changes. It does not > seem any cleaner, but it may performance better. True, this would not be any cleaner. > 2) Stop using IFRAME all together. This is probably the best option in the > long run, but it is not a trial task and I am not sure it is possible given > that we are using fixed divs for scrolling. This would not entirely solve the issue because integrators of Orion might put it in hidden elements, in hidden iframes and so on. (see details below) > 3) Do not handle this at all and let the application code only create the text > view when the parent is made visible (display:block). This just shift the > burden to the app code. > > 4) Fix mozilla's bug https://bugzilla.mozilla.org/show_bug.cgi?id=548397 . > > Can you think of any other solution? I would advocate for solution no. 3: do not handle this at all. I might be mistaken, but here's my reasoning: the current solution is probably too generic and prone to errors. While moving the burden from Orion to the integrator sounds like just avoiding problems here, for us, in Orion, it's actually much simpler for the integrator to solve the problem. The best solution is always very much dependent on how the integrator chooses to use Orion. Beyond the above reasoning, the current solution (DOMAttrModified), for example, doesn't work in how we integrate Orion into Firefox, for two reasons: (1) We had to wrap Orion in an iframe and the Orion code only checks the parent document - the DOMAttrModified event listener is not added to the entire tree of parent documents. So, Orion's own iframe is always "visible" in our iframe. (2) We never have the Orion editor inside a hidden element. In the TextView _getVisible() terms, Orion is not "hidden" or "disconnected". The problem with getComputedStyle() failing still occurred for us, even if we always have the editor visible (including our wrapping iframe). It is interesting to note that async initialization shouldn't be solely about the problem of postponing initialization until the editor is visible. My thoughts are more related to how the iframe is loaded, how the XHR happens (the sync XHR freezes the UI until it completes), and so on. Nonetheless, even my initial bug report points out to the problem of the editor visibility and how computed styles fail to work - which is dismaying per se. Now I believe that the problems I had were caused by hitting the Mozilla bug 548397 due to initialization happening "too fast". In our usage the Orion editor is never hidden - it's always visible as pointed out - but initialization happens during window opens, and, most-likely computed style information is not yet available at execution time. We see these problems most often when we run our automated tests (which always run "as fast as possible"). Getting back to the async initialization: in our use-case I believe that by properly listening for the iframe load + the async XHR would give sufficient time for computed styles info to become available. I would still like to see this kind of async init implemented somehow in Orion. Going further, bug 337160 comment 12 points out important aspects: > If the editor becomes hidden (or disconnected) after its creation, > the iframe contents is unload. It will be loaded again if the editor is > shown (connect). This is something that the integrator should have control of. Creating and destroying the view is a costly operation. If I do not want to do anything with the editor while it's hidden, I want to be able to get back to it, where it was, without the UI being rebuilt/recreated. > During the time the editor contents is not loaded, any APIs in the editor that > require the DOM will have undefined behavior. They might throw exceptions. This is certainly not ideal. I should be able to rely on the editor API to work after explicit initialization until a subsequent explicit request to destroy it. > There are APIs that are valid while the editor is not loaded. For example: > [...] In the list of APIs I see, for example, setText() but one could have Orion event handlers that rely on the DOM API, that can fail. This brings me to my main point (and proposal: TextView should be split into two separate objects. (1) TextEditor that is always-sync initialized, it has no UI and it is the main "driver" of the whole editor API. It would have setText()/getText(), selection, caret, actions and more API. It would have its own events, like it does now. (2) TextView which is always-async initialized. This would create the UI, the iframe, and it would handle rulers and whatever is UI related, including keybindings. It would take a TextEditor instance to be a view of. What would the above allow us to do? We could create TextEditor instances without any UI and we could have only one TextView instance for all of the editors. It would be far less confusing for users of the API to know which methods rely on the DOM or not. If the editor has no view, we can avoid calling methods from the view and so on. We could have view.setEditor(), so in a tabbed-editor we could have only one view for all editors and just switch like this. Or we could have one view per editor, and when the view is hidden we can let it hidden without destroying it (or we can destroy views whenever needed, it would be up to us, in our control... as integrators). The current TextView situation is mixed and it really stems from the fact that TextView is both a view and an editor state handler. (For example, in one of the projects where we use Orion we ended up having to maintain some of the editor state in our objects, because we needed to allow the user to switch between editing multiple CSS files.) For this bug I would just limit things to adding async init to Orion. The above idea is certainly too ambitious, too far outreaching (for now), but I believe it's worth keeping it in mind. Comments are welcome. Thank you! (apologies for my really long comment!)
>For this bug I would just limit things to adding async init to Orion. The above >idea is certainly too ambitious, too far outreaching (for now), but I believe >it's worth keeping it in mind. We have added async creation support including the onLoad and onUnload events. There is an option flag if the app code desires to switch sync load back on. The default is false. In the async case, we used the window load event to decide when the iframe is ready to create its contents (frame load happens to early before the style sheets are loaded). The inline style sheets were replaced be link elements in the async case. http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=ef0e6327393ecc4b24c539c9c2489273134bdf25 I would suggest opening a new bug for your other idea (TextEditor/TextView). I believe that editor.js could be non UI version and textView.js the UI version. It certainly can be implemented on top of textView.js as is.
