Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
408783002:
Enable Try Dart to run on IE11. (Closed)
DescriptionMade couple tweaks and added several options to allow Try Dart to run in IE11.
BUG=dartbug.com/20017
R=ahe@google.com
Committed: https://code.google.com/p/dart/source/detail?r=39394
Patch Set 1 #
Total comments: 19
Patch Set 2 : Clean up formatting errors, added test. #Patch Set 3 : An attempt to get rid of exceptions, explicit options to handle IE11 #Patch Set 4 : Reverted test, changed isBlockElement to accomodate for display=='none' #Patch Set 5 : Added 'diagnostic' class to wrapper span, narrowed hover css application #
Total comments: 10
Patch Set 6 : #
Total comments: 2
Patch Set 7 : removed dynamic, marked one test as Fail on IE11 #
Total comments: 3
Messages
Total messages: 25 (0 generated)
Hi Peter, wanted to share with you what I've done to make Try Dart work on IE11(still works on Chrome, FF). What do you think? Thanks!
https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart File site/try/src/html_to_text.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { Reason for this change is that diagnostic messages are block elements in IE(IE doesn't support shadow nodes). So '\n' are being inserted as you edit through the diagnostic message. I could not clearly see the use case for adding '\n'(copy/pasting rich-text source code?). So I adjusted this as well as the test.
Very nice, as always :-) I have two concerns: 1. The change to treat root as inline by default. I'll have to look more at that. 2. If I understand correctly, an IE user would have to set alwaysRunInIframe and communicateViaBlobs manually in the settings menu. Is there a way to automatically detect that we're running on IE and set them by default? I'm taking vacation this week, so I may not respond until next week. https://codereview.chromium.org/408783002/diff/1/site/try/src/compiler_isolat... File site/try/src/compiler_isolate.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/compiler_isolat... site/try/src/compiler_isolate.dart:19: const bool THROW_ON_ERROR = false; Undo this :-) https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart File site/try/src/html_to_text.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { On 2014/07/21 01:38:28, aam wrote: > Reason for this change is that diagnostic messages are block elements in IE(IE > doesn't support shadow nodes). So '\n' are being inserted as you edit through > the diagnostic message. > I could not clearly see the use case for adding '\n'(copy/pasting rich-text > source code?). So I adjusted this as well as the test. IIRC, this code is to support copy and paste from, for example, Google Docs in Chrome. I'm not sure about this change. We should have removed the diagnostics before getting to this point. I can't find the "adjusted test". https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... site/try/src/interaction_manager.dart:480: if (containsNode(mainEditorPane, trySelection.anchorNode)) { Indentation. https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... site/try/src/interaction_manager.dart:1241: Remove line. https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... site/try/src/interaction_manager.dart:1320: // above is for Firefox, IE doesn't have problems with anchorOffset. Can we detect that selection.modify doesn't work and avoid throwing an exception? Throwing exceptions makes debugging harder. https://codereview.chromium.org/408783002/diff/1/site/try/src/settings.dart File site/try/src/settings.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/settings.dart#n... site/try/src/settings.dart:19: const BooleanUserOption('alwaysRunInIframe'); Can we make these settings hidden and automatically set them? https://codereview.chromium.org/408783002/diff/1/site/try/src/settings.dart#n... site/try/src/settings.dart:21: bool get alwaysRunInIframe => _alwaysRunInIframe.value; Since this is a new option, I prefer not adding the getters and setters (use .value instead). https://codereview.chromium.org/408783002/diff/1/site/try/src/shadow_root.dart File site/try/src/shadow_root.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/shadow_root.dar... site/try/src/shadow_root.dart:105: Extra line.
Thank you Peter for taking time to respond to this CL! Areas of concern you identified make perfect sense, will look into those closer. If not this week(next few days will be busy), then next. https://codereview.chromium.org/408783002/diff/1/site/try/src/compiler_isolat... File site/try/src/compiler_isolate.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/compiler_isolat... site/try/src/compiler_isolate.dart:19: const bool THROW_ON_ERROR = false; On 2014/07/21 08:15:59, ahe wrote: > Undo this :-) Done. https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart File site/try/src/html_to_text.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { >I'm not sure about this change. We should have removed the >diagnostics before getting to this point. Interesting. I will look into this. > I can't find the "adjusted test". Ah, forgot to upload changelist with updated tests\try\web\paste_content_rewriting_test.dart. Added the test now. https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... site/try/src/interaction_manager.dart:480: if (containsNode(mainEditorPane, trySelection.anchorNode)) { On 2014/07/21 08:16:00, ahe wrote: > Indentation. Done. https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... site/try/src/interaction_manager.dart:1241: On 2014/07/21 08:16:00, ahe wrote: > Remove line. Done. https://codereview.chromium.org/408783002/diff/1/site/try/src/interaction_man... site/try/src/interaction_manager.dart:1320: // above is for Firefox, IE doesn't have problems with anchorOffset. On 2014/07/21 08:16:00, ahe wrote: > Can we detect that selection.modify doesn't work and avoid throwing an > exception? Throwing exceptions makes debugging harder. Acknowledged. https://codereview.chromium.org/408783002/diff/1/site/try/src/settings.dart File site/try/src/settings.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/settings.dart#n... site/try/src/settings.dart:19: const BooleanUserOption('alwaysRunInIframe'); On 2014/07/21 08:16:00, ahe wrote: > Can we make these settings hidden and automatically set them? Acknowledged. https://codereview.chromium.org/408783002/diff/1/site/try/src/settings.dart#n... site/try/src/settings.dart:21: bool get alwaysRunInIframe => _alwaysRunInIframe.value; On 2014/07/21 08:16:00, ahe wrote: > Since this is a new option, I prefer not adding the getters and setters (use > .value instead). Acknowledged. https://codereview.chromium.org/408783002/diff/1/site/try/src/shadow_root.dart File site/try/src/shadow_root.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/shadow_root.dar... site/try/src/shadow_root.dart:105: On 2014/07/21 08:16:00, ahe wrote: > Extra line. Done.
Hi Peter, please take a look when you have a chance. Thanks! https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart File site/try/src/html_to_text.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { On 2014/07/21 12:20:38, aam wrote: > >I'm not sure about this change. We should have removed the >diagnostics before > getting to this point. > Interesting. I will look into this. htmlToText is invoked from normalizeMutationRecord function, which receives mutation records. In IE mutation records come back with diagnostic spans. For example, typing some invalid characters 'ajs' at the end of the line "void main() { ajs" results in IE: Single line change: <span class="lineNumber" dart-state="{"><span style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span style="color: rgb(0, 0, 0);"><a class="diagnostic">ajs<span style="font-weight: normal;"><span try-dart-shadow-root=""><span class="alert alert-error" style="font-weight: normal; display: block;"><span try-dart-shadow-root="">Expected ';' after this.</span></span><span class="alert" style="font-weight: normal; display: block;"><span try-dart-shadow-root="">Cannot resolve 'aj'.</span></span></span></span></a></span> in Chrome: Single line change: <span dart-state="{" class="lineNumber"><span style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span style="color: rgb(0, 0, 0);">ajs</span> </span> And in IE this diagnostic element: "<span try-dart-shadow-root=\"\"><span class=\"alert alert-error\" style=\"font-weight: normal; display: block;\"><span try-dart-shadow-root=\"\">Expected ';' after this.</span></span><span class=\"alert\" style=\"font-weight: normal; display: block;\"><span try-dart-shadow-root=\"\">Cannot resolve 'ajsd'.</span></span></span>" triggers 'buffer.write('\n');' at line 72 below. Which results in additional newline character added after line "void main() { ajs".
Thank you for investigating! https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart File site/try/src/html_to_text.dart (right): https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { On 2014/07/30 00:28:25, aam wrote: > On 2014/07/21 12:20:38, aam wrote: > > >I'm not sure about this change. We should have removed the >diagnostics > before > > getting to this point. > > Interesting. I will look into this. > > htmlToText is invoked from normalizeMutationRecord function, which receives > mutation records. In IE mutation records come back with diagnostic spans. For > example, typing some invalid characters 'ajs' at the end of the line "void > main() { ajs" results > > in IE: Single line change: <span class="lineNumber" dart-state="{"><span > style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > style="color: rgb(0, 0, 0);"><a class="diagnostic">ajs<span style="font-weight: > normal;"><span try-dart-shadow-root=""><span class="alert alert-error" > style="font-weight: normal; display: block;"><span > try-dart-shadow-root="">Expected ';' after this.</span></span><span > class="alert" style="font-weight: normal; display: block;"><span > try-dart-shadow-root="">Cannot resolve > 'aj'.</span></span></span></span></a></span> > > in Chrome: > Single line change: <span dart-state="{" class="lineNumber"><span style="color: > rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > style="color: rgb(0, 0, 0);">ajs</span> > </span> > > And in IE this diagnostic element: > > "<span try-dart-shadow-root=\"\"><span class=\"alert alert-error\" > style=\"font-weight: normal; display: block;\"><span > try-dart-shadow-root=\"\">Expected ';' after this.</span></span><span > class=\"alert\" style=\"font-weight: normal; display: block;\"><span > try-dart-shadow-root=\"\">Cannot resolve 'ajsd'.</span></span></span>" > > triggers 'buffer.write('\n');' at line 72 below. Which results in additional > newline character added after line "void main() { ajs". I noticed something troublesome in normalizeMutationRecord: we store line containing the change in normalizedNodes, but then when we normalize the node afterwards, we operate directly on "node", not the line. I suspect that is broken. Also, if you take a look at walkNodes in walkNodes in shadow_root.dart, the diagnostic element is supposed to be skipped because it has the attribute "try-dart-shadow-root". So something else is wrong, the closure on line 46 isn't supposed to be invoked with a diagnostic element. Could you debug this aspect and see if there is a bug in walkNodes?
On 2014/08/04 08:59:55, ahe wrote: > Thank you for investigating! > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart > File site/try/src/html_to_text.dart (right): > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... > site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { > On 2014/07/30 00:28:25, aam wrote: > > On 2014/07/21 12:20:38, aam wrote: > > > >I'm not sure about this change. We should have removed the >diagnostics > > before > > > getting to this point. > > > Interesting. I will look into this. > > > > htmlToText is invoked from normalizeMutationRecord function, which receives > > mutation records. In IE mutation records come back with diagnostic spans. For > > example, typing some invalid characters 'ajs' at the end of the line "void > > main() { ajs" results > > > > in IE: Single line change: <span class="lineNumber" dart-state="{"><span > > style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, > 0, > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > style="color: rgb(0, 0, 0);"><a class="diagnostic">ajs<span > style="font-weight: > > normal;"><span try-dart-shadow-root=""><span class="alert alert-error" > > style="font-weight: normal; display: block;"><span > > try-dart-shadow-root="">Expected ';' after this.</span></span><span > > class="alert" style="font-weight: normal; display: block;"><span > > try-dart-shadow-root="">Cannot resolve > > 'aj'.</span></span></span></span></a></span> > > > > in Chrome: > > Single line change: <span dart-state="{" class="lineNumber"><span > style="color: > > rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > style="color: rgb(0, 0, 0);">ajs</span> > > </span> > > > > And in IE this diagnostic element: > > > > "<span try-dart-shadow-root=\"\"><span class=\"alert alert-error\" > > style=\"font-weight: normal; display: block;\"><span > > try-dart-shadow-root=\"\">Expected ';' after this.</span></span><span > > class=\"alert\" style=\"font-weight: normal; display: block;\"><span > > try-dart-shadow-root=\"\">Cannot resolve 'ajsd'.</span></span></span>" > > > > triggers 'buffer.write('\n');' at line 72 below. Which results in additional > > newline character added after line "void main() { ajs". > > I noticed something troublesome in normalizeMutationRecord: we store line > containing the change in normalizedNodes, but then when we normalize the node > afterwards, we operate directly on "node", not the line. I suspect that is > broken. Are you talking about handling of record.addedNodes? In my particular scenario(editing one line causes new line break > Also, if you take a look at walkNodes in walkNodes in shadow_root.dart, the > diagnostic element is supposed to be skipped because it has the attribute > "try-dart-shadow-root". So something else is wrong, the closure on line 46 isn't > supposed to be invoked with a diagnostic element. Could you debug this aspect > and see if there is a bug in walkNodes? walkNodes does seem to have a problem with invoking line 46 closure at line 91 "int action = f(node);", after "try-dart-shadow-root" spans are skipped at line 89 "skip(node, walker);". Seems that "int action = f(node);" should go into "else" branch. But actually invokation of line 46 closure is not a problem - it is "buffer.write('\n');" that happens in htmlToText line 72 after walkNodes is done, which causes issues.
On 2014/08/04 08:59:55, ahe wrote: > Thank you for investigating! > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart > File site/try/src/html_to_text.dart (right): > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... > site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { > On 2014/07/30 00:28:25, aam wrote: > > On 2014/07/21 12:20:38, aam wrote: > > > >I'm not sure about this change. We should have removed the >diagnostics > > before > > > getting to this point. > > > Interesting. I will look into this. > > > > htmlToText is invoked from normalizeMutationRecord function, which receives > > mutation records. In IE mutation records come back with diagnostic spans. For > > example, typing some invalid characters 'ajs' at the end of the line "void > > main() { ajs" results > > > > in IE: Single line change: <span class="lineNumber" dart-state="{"><span > > style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, > 0, > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > style="color: rgb(0, 0, 0);"><a class="diagnostic">ajs<span > style="font-weight: > > normal;"><span try-dart-shadow-root=""><span class="alert alert-error" > > style="font-weight: normal; display: block;"><span > > try-dart-shadow-root="">Expected ';' after this.</span></span><span > > class="alert" style="font-weight: normal; display: block;"><span > > try-dart-shadow-root="">Cannot resolve > > 'aj'.</span></span></span></span></a></span> > > > > in Chrome: > > Single line change: <span dart-state="{" class="lineNumber"><span > style="color: > > rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > style="color: rgb(0, 0, 0);">ajs</span> > > </span> > > > > And in IE this diagnostic element: > > > > "<span try-dart-shadow-root=\"\"><span class=\"alert alert-error\" > > style=\"font-weight: normal; display: block;\"><span > > try-dart-shadow-root=\"\">Expected ';' after this.</span></span><span > > class=\"alert\" style=\"font-weight: normal; display: block;\"><span > > try-dart-shadow-root=\"\">Cannot resolve 'ajsd'.</span></span></span>" > > > > triggers 'buffer.write('\n');' at line 72 below. Which results in additional > > newline character added after line "void main() { ajs". > > I noticed something troublesome in normalizeMutationRecord: we store line > containing the change in normalizedNodes, but then when we normalize the node > afterwards, we operate directly on "node", not the line. I suspect that is > broken. Are you talking about handling of record.addedNodes? In my particular scenario(editing one line causes new line break > Also, if you take a look at walkNodes in walkNodes in shadow_root.dart, the > diagnostic element is supposed to be skipped because it has the attribute > "try-dart-shadow-root". So something else is wrong, the closure on line 46 isn't > supposed to be invoked with a diagnostic element. Could you debug this aspect > and see if there is a bug in walkNodes? walkNodes does seem to have a problem with invoking line 46 closure at line 91 "int action = f(node);", after "try-dart-shadow-root" spans are skipped at line 89 "skip(node, walker);". Seems that "int action = f(node);" should go into "else" branch. But actually invokation of line 46 closure is not a problem - it is "buffer.write('\n');" that happens in htmlToText line 72 after walkNodes is done, which causes issues.
On 2014/08/04 08:59:55, ahe wrote: > Thank you for investigating! > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart > File site/try/src/html_to_text.dart (right): > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... > site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { > On 2014/07/30 00:28:25, aam wrote: > > On 2014/07/21 12:20:38, aam wrote: > > > >I'm not sure about this change. We should have removed the >diagnostics > > before > > > getting to this point. > > > Interesting. I will look into this. > > > > htmlToText is invoked from normalizeMutationRecord function, which receives > > mutation records. In IE mutation records come back with diagnostic spans. For > > example, typing some invalid characters 'ajs' at the end of the line "void > > main() { ajs" results > > > > in IE: Single line change: <span class="lineNumber" dart-state="{"><span > > style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, > 0, > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > style="color: rgb(0, 0, 0);"><a class="diagnostic">ajs<span > style="font-weight: > > normal;"><span try-dart-shadow-root=""><span class="alert alert-error" > > style="font-weight: normal; display: block;"><span > > try-dart-shadow-root="">Expected ';' after this.</span></span><span > > class="alert" style="font-weight: normal; display: block;"><span > > try-dart-shadow-root="">Cannot resolve > > 'aj'.</span></span></span></span></a></span> > > > > in Chrome: > > Single line change: <span dart-state="{" class="lineNumber"><span > style="color: > > rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span style="color: > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > style="color: rgb(0, 0, 0);">ajs</span> > > </span> > > > > And in IE this diagnostic element: > > > > "<span try-dart-shadow-root=\"\"><span class=\"alert alert-error\" > > style=\"font-weight: normal; display: block;\"><span > > try-dart-shadow-root=\"\">Expected ';' after this.</span></span><span > > class=\"alert\" style=\"font-weight: normal; display: block;\"><span > > try-dart-shadow-root=\"\">Cannot resolve 'ajsd'.</span></span></span>" > > > > triggers 'buffer.write('\n');' at line 72 below. Which results in additional > > newline character added after line "void main() { ajs". > > I noticed something troublesome in normalizeMutationRecord: we store line > containing the change in normalizedNodes, but then when we normalize the node > afterwards, we operate directly on "node", not the line. I suspect that is > broken. Are you talking about handling of record.addedNodes in lines 1224-1232? I'm not sure - will we see problems will with highlighting because of that(normalizedNodes collection seems to be used only for highlighting)? In my particular scenario(editing one line with diagnostics causes new line breaks added) addedNodes is empty. Only record.target is populated. > Also, if you take a look at walkNodes in walkNodes in shadow_root.dart, the > diagnostic element is supposed to be skipped because it has the attribute > "try-dart-shadow-root". So something else is wrong, the closure on line 46 isn't > supposed to be invoked with a diagnostic element. Could you debug this aspect > and see if there is a bug in walkNodes? walkNodes does seem to have a problem with invoking line 46 closure at line 91 "int action = f(node);", after "try-dart-shadow-root" spans are skipped at line 89 "skip(node, walker);". Seems that "int action = f(node);" should go into "else" branch. But from what I see such invocation of "line 46"-closure is not a problem - it is "buffer.write('\n');" that happens in htmlToText line 72 _after_ walkNodes is done, which causes issues with extra newline records added.
On 2014/08/04 11:34:55, aam wrote: > On 2014/08/04 08:59:55, ahe wrote: > > Thank you for investigating! > > > > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.dart > > File site/try/src/html_to_text.dart (right): > > > > > https://codereview.chromium.org/408783002/diff/1/site/try/src/html_to_text.da... > > site/try/src/html_to_text.dart:44: {bool treatRootAsInline: true}) { > > On 2014/07/30 00:28:25, aam wrote: > > > On 2014/07/21 12:20:38, aam wrote: > > > > >I'm not sure about this change. We should have removed the >diagnostics > > > before > > > > getting to this point. > > > > Interesting. I will look into this. > > > > > > htmlToText is invoked from normalizeMutationRecord function, which receives > > > mutation records. In IE mutation records come back with diagnostic spans. > For > > > example, typing some invalid characters 'ajs' at the end of the line "void > > > main() { ajs" results > > > > > > in IE: Single line change: <span class="lineNumber" dart-state="{"><span > > > style="color: rgb(126, 8, 84);"><b>void</b></span> <span style="color: > rgb(0, > > 0, > > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span > style="color: > > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > > style="color: rgb(0, 0, 0);"><a class="diagnostic">ajs<span > > style="font-weight: > > > normal;"><span try-dart-shadow-root=""><span class="alert alert-error" > > > style="font-weight: normal; display: block;"><span > > > try-dart-shadow-root="">Expected ';' after this.</span></span><span > > > class="alert" style="font-weight: normal; display: block;"><span > > > try-dart-shadow-root="">Cannot resolve > > > 'aj'.</span></span></span></span></a></span> > > > > > > in Chrome: > > > Single line change: <span dart-state="{" class="lineNumber"><span > > style="color: > > > rgb(126, 8, 84);"><b>void</b></span> <span style="color: rgb(0, 0, > > > 0);">main</span><span style="color: rgb(0, 0, 0);">(</span><span > style="color: > > > rgb(0, 0, 0);">)</span> <span style="color: rgb(0, 0, 0);">{</span> <span > > > style="color: rgb(0, 0, 0);">ajs</span> > > > </span> > > > > > > And in IE this diagnostic element: > > > > > > "<span try-dart-shadow-root=\"\"><span class=\"alert alert-error\" > > > style=\"font-weight: normal; display: block;\"><span > > > try-dart-shadow-root=\"\">Expected ';' after this.</span></span><span > > > class=\"alert\" style=\"font-weight: normal; display: block;\"><span > > > try-dart-shadow-root=\"\">Cannot resolve 'ajsd'.</span></span></span>" > > > > > > triggers 'buffer.write('\n');' at line 72 below. Which results in additional > > > newline character added after line "void main() { ajs". > > > > I noticed something troublesome in normalizeMutationRecord: we store line > > containing the change in normalizedNodes, but then when we normalize the node > > afterwards, we operate directly on "node", not the line. I suspect that is > > broken. > > Are you talking about handling of record.addedNodes in lines 1224-1232? I'm not > sure - will we see problems will with highlighting because of > that(normalizedNodes collection seems to be used only for highlighting)? > In my particular scenario(editing one line with diagnostics causes new line > breaks added) addedNodes is empty. Only record.target is populated. I see. It was more a general observation about something I noticed when looking at the code. I don't think it is related to the problem you see, but I may be able to construct another situation where it is a problem. For example, if mutations are batched up. > > Also, if you take a look at walkNodes in walkNodes in shadow_root.dart, the > > diagnostic element is supposed to be skipped because it has the attribute > > "try-dart-shadow-root". So something else is wrong, the closure on line 46 > isn't > > supposed to be invoked with a diagnostic element. Could you debug this aspect > > and see if there is a bug in walkNodes? > > walkNodes does seem to have a problem with invoking line 46 closure at line 91 > "int action = f(node);", after "try-dart-shadow-root" spans are skipped at line > 89 "skip(node, walker);". Seems that "int action = f(node);" should go into > "else" branch. I think you're right about that. > But from what I see such invocation of "line 46"-closure is not a problem - it > is "buffer.write('\n');" that happens in htmlToText line 72 _after_ walkNodes is > done, which causes issues with extra newline records added. Yes. I understand how "buffer.write('\n');" seems to be a problem here, but I think the real problem is that htmlToText calls itself recursively on the diagnostic node. That shouldn't happen to begin with, and I hypothesize that if you can avoid that call you have fixed the problem.
On 2014/08/04 11:44:53, ahe wrote:
> Yes. I understand how "buffer.write('\n');" seems to be a problem here, but I
> think the real problem is that htmlToText calls itself recursively on the
> diagnostic node. That shouldn't happen to begin with, and I hypothesize that
if
> you can avoid that call you have fixed the problem.
I see.
In IE top diagnostic span(at line 3 below) doesn't have "try-dart-shadow-root".
Chrome doesn't have any spans visible in DOM model following the 'ajsd' text. I
will look into how to add try-dart-shadow-root attribute to that span.
1: <span style=\"color: rgb(0, 0, 0);\">
2: <a class=\"diagnostic\">ajsd
3: <span style=\"font-weight: normal;\">
4: <span try-dart-shadow-root=\"\">
5: <span class=\"alert alert-error\" style=\"font-weight: normal;
display: block;\">
6: <span try-dart-shadow-root=\"\">Expected ';' after this.</span>
7: </span>
8: <span class=\"alert\" style=\"font-weight: normal; display: block;\">
9: <span try-dart-shadow-root=\"\">Cannot resolve 'ajsdh'.</span>
a: </span>
b: </span>
c: </span>
d: </a>
e:</span>
On 2014/08/04 12:11:33, aam wrote:
> In IE top diagnostic span(at line 3 below) doesn't have
"try-dart-shadow-root".
> Chrome doesn't have any spans visible in DOM model following the 'ajsd' text.
I
> will look into how to add try-dart-shadow-root attribute to that span.
I have a new theory: <span style=\"font-weight: normal;\"> is added by IE's
support for contenteditable, and the element is in "hover" mode. If so, this
rule (from index.html) applies:
a:hover.diagnostic>span {
display: block;
position: absolute;
/* left: 1em; */
/* top: 2em; */
right: 1%;
}
So the span element is indeed a block element. If I'm right, then we need to
handle diagnostic elements differently, for example, in htmlToText, after these
two lines
if (node.nodeName == 'BR') {
buffer.write('\n');
Add:
} else if (node.classes.contains("diagnostic")) {
// Some kind of magic.
I'm not sure what kind of magic is needed here, and I'll ponder it some more.
Perhaps you have an idea?
Cheers,
Peter
On 2014/08/04 14:21:42, ahe wrote:
> I have a new theory: <span style=\"font-weight: normal;\"> is added by IE's
> support for contenteditable, and the element is in "hover" mode.
This span element seems to be added by addDiagnostic in editor.dart:169-189.
> If so, this rule (from index.html) applies:
>
> a:hover.diagnostic>span {
> display: block;
> position: absolute;
> /* left: 1em; */
> /* top: 2em; */
> right: 1%;
> }
>
> So the span element is indeed a block element.
Oh... Actually, style["display"] for this span is "none". isBlockElement
checks value for !== "inline", which makes this span "block element".
I did not realize importance of being a block element. Why are they special?
Anyway, I changed isBlockElement to check (display != 'inline' && display !=
'none') and the problem with inserted '\n' is solved now!
I uploaded updated patch.
What do you think?
Thanks,
Alexander.
On 2014/08/05 01:50:23, aam wrote: > Oh... Actually, style["display"] for this span is "none". isBlockElement > checks value for !== "inline", which makes this span "block element". > I did not realize importance of being a block element. Why are they special? This has to do with copy and paste from a webpage. See this bug: https://code.google.com/p/dart/issues/detail?id=17726. > Anyway, I changed isBlockElement to check (display != 'inline' && display != > 'none') and the problem with inserted '\n' is solved now! That makes sense. I think you should keep the check for 'none', but the diagnostic mechanism feels brittle. How about changing the CSS and addDiagnostic so that the nested span also have the class "diagnostic"? That is, change index.html so the selector is "a:hover.diagnostic>span.diagnostic", and make sure a diagnostic looks like this: <a class="diagnostic">ajds<span class="diagnostic">...</span></a> > > I uploaded updated patch. > > What do you think? > > Thanks, > Alexander.
On 2014/08/05 15:10:30, ahe wrote: > > I did not realize importance of being a block element. Why are they special? > This has to do with copy and paste from a webpage. See this bug: > https://code.google.com/p/dart/issues/detail?id=17726. Ah, I see. > > Anyway, I changed isBlockElement to check (display != 'inline' && display != > > 'none') and the problem with inserted '\n' is solved now! > That makes sense. I think you should keep the check for 'none', but the > diagnostic mechanism feels brittle. How about changing the CSS and addDiagnostic > so that the nested span also have the class "diagnostic"? > That is, change index.html so the selector is > "a:hover.diagnostic>span.diagnostic", and make sure a diagnostic looks like > this: > <a class="diagnostic">ajds<span class="diagnostic">...</span></a> I have done that and then noticed that there are also spans with class "alert" that should also become visible on hover. So I added a:hover.diagnostic>span.alert to css style in index.hmtl as well. Please let me know what you think. Thanks!
Hi, Peter, just wanted to ping you regarding this CL - whether it's something worth getting across the finish line at this point :-) Thanks, Alexander Aprelev.
LGTM! Can you enable Try Dart tests on IE11 now? See tests/try/try.status. https://codereview.chromium.org/408783002/diff/120001/site/try/src/interactio... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/interactio... site/try/src/interaction_manager.dart:1313: var selectionProxy = new hack.JsObject.fromBrowserObject(selection); Perhaps we can find a better way to encapsulate this. Could you add a todo for me to come up with a way to better encapsulate this method? https://codereview.chromium.org/408783002/diff/120001/site/try/src/leap.dart File site/try/src/leap.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/leap.dart#... site/try/src/leap.dart:43: alwaysRunInIframe.value = isBrowserIE(); Can we make it so that this is a default? I'd like to be able to write something like this in a JavaScript console: localStorage.alwaysRunInIframe = true And the code would always run in an iframe regardless of the browser. https://codereview.chromium.org/408783002/diff/120001/site/try/src/settings.dart File site/try/src/settings.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/settings.d... site/try/src/settings.dart:116: _currentSample, Add the new options to this list. https://codereview.chromium.org/408783002/diff/120001/site/try/src/shadow_roo... File site/try/src/shadow_root.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/shadow_roo... site/try/src/shadow_root.dart:53: /// so this is brute-force implementation of contains. so this is *a* ... https://codereview.chromium.org/408783002/diff/120001/site/try/src/shadow_roo... site/try/src/shadow_root.dart:55: var p = child; Indentation.
Thanks for the review Peter! Should be all done. Please take a look at setting of default values for those IE-specific options when you have a chance, whether that is what you had in mind?
https://codereview.chromium.org/408783002/diff/120001/site/try/src/interactio... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/interactio... site/try/src/interaction_manager.dart:1313: var selectionProxy = new hack.JsObject.fromBrowserObject(selection); On 2014/08/19 09:22:30, ahe wrote: > Perhaps we can find a better way to encapsulate this. > > Could you add a todo for me to come up with a way to better encapsulate this > method? Done. https://codereview.chromium.org/408783002/diff/120001/site/try/src/leap.dart File site/try/src/leap.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/leap.dart#... site/try/src/leap.dart:43: alwaysRunInIframe.value = isBrowserIE(); On 2014/08/19 09:22:30, ahe wrote: > Can we make it so that this is a default? > > I'd like to be able to write something like this in a JavaScript console: > > localStorage.alwaysRunInIframe = true > > And the code would always run in an iframe regardless of the browser. Done. https://codereview.chromium.org/408783002/diff/120001/site/try/src/settings.dart File site/try/src/settings.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/settings.d... site/try/src/settings.dart:116: _currentSample, On 2014/08/19 09:22:30, ahe wrote: > Add the new options to this list. Done. https://codereview.chromium.org/408783002/diff/120001/site/try/src/shadow_roo... File site/try/src/shadow_root.dart (right): https://codereview.chromium.org/408783002/diff/120001/site/try/src/shadow_roo... site/try/src/shadow_root.dart:53: /// so this is brute-force implementation of contains. On 2014/08/19 09:22:30, ahe wrote: > so this is *a* ... Done. https://codereview.chromium.org/408783002/diff/120001/site/try/src/shadow_roo... site/try/src/shadow_root.dart:55: var p = child; On 2014/08/19 09:22:30, ahe wrote: > Indentation. Done.
LGTM! Awesome, thank you. https://codereview.chromium.org/408783002/diff/140001/site/try/src/user_optio... File site/try/src/user_option.dart (right): https://codereview.chromium.org/408783002/diff/140001/site/try/src/user_optio... site/try/src/user_option.dart:39: void setIfNotInitialized(dynamic newValueEvaluator()) { Remove "dynamic". https://codereview.chromium.org/408783002/diff/140001/tests/try/try.status File tests/try/try.status (right): https://codereview.chromium.org/408783002/diff/140001/tests/try/try.status#ne... tests/try/try.status:9: [ $compiler != dart2js || ($runtime != drt && $runtime != chrome && $runtime != ff && $runtime != safari && $runtime != ie11) ] \o/
On 2014/08/19 13:02:49, ahe wrote: > https://codereview.chromium.org/408783002/diff/140001/tests/try/try.status#ne... > tests/try/try.status:9: [ $compiler != dart2js || ($runtime != drt && $runtime > != chrome && $runtime != ff && $runtime != safari && $runtime != ie11) ] > \o/ One test is failing. I opened dartbug.com/20593 for that, will continue looking into that.
Message was sent while issue was closed.
Committed patchset #7 manually as r39394 (presubmit successful).
Message was sent while issue was closed.
Hi Peter, change below must have caused a regress on Firefox as I broke workAroundFirefoxBug. Please take a look at the comment if it makes sense. Thanks! https://codereview.chromium.org/408783002/diff/160001/site/try/src/interactio... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/160001/site/try/src/interactio... site/try/src/interaction_manager.dart:1316: if (modify != null) { This doesn't work as it should. Selection js object is not being recognized by _convertToJS(js_dart2js.dart:504) as straight-through objects - it's not in the list below: === } else if (o is Blob || o is Event || o is KeyRange || o is ImageData || o is Node || o is TypedData || o is Window) { return o; === instead it is handled as generic js object in last else: === } else { var ctor = _dartProxyCtor; return _getJsProxy(o, _JS_OBJECT_PROPERTY_NAME, (o) => JS('', 'new #(#)', ctor, o)); } === and as a result selectionProxy here is not actual javascript selection object, but JsProxy. Not sure if there is a way to get through the JsProxy to javascript object. Adding "|| o is Selection" to the condition above seem to do the job.
Message was sent while issue was closed.
https://codereview.chromium.org/408783002/diff/160001/site/try/src/interactio... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/160001/site/try/src/interactio... site/try/src/interaction_manager.dart:1316: if (modify != null) { On 2014/08/29 14:34:45, aam wrote: > This doesn't work as it should. > Selection js object is not being recognized by _convertToJS(js_dart2js.dart:504) > as straight-through objects - it's not in the list below: > === > } else if (o is Blob || o is Event || o is KeyRange || o is ImageData || o is > Node || o is TypedData || o is Window) { > return o; > === > > instead it is handled as generic js object in last else: > === > } else { > var ctor = _dartProxyCtor; > return _getJsProxy(o, _JS_OBJECT_PROPERTY_NAME, > (o) => JS('', 'new #(#)', ctor, o)); > } > === > and as a result selectionProxy here is not actual javascript selection object, > but JsProxy. > > Not sure if there is a way to get through the JsProxy to javascript object. > Adding "|| o is Selection" to the condition above seem to do the job. I think this works because Selection isn't being passed straight through. What problem are you seeing?
Message was sent while issue was closed.
https://codereview.chromium.org/408783002/diff/160001/site/try/src/interactio... File site/try/src/interaction_manager.dart (right): https://codereview.chromium.org/408783002/diff/160001/site/try/src/interactio... site/try/src/interaction_manager.dart:1316: if (modify != null) { So the problem is that on Firefox this [modify] variable is null, therefore code below that adjusts selection is not triggered. I took this logic from [installErrorHandler] from native_helper.dart, but in [installErrorHandler]'s case iframe is a Node, which is passed through: === void installErrorHandler() { ... var iframeProxy = new hack.JsObject.fromBrowserObject(iframe); var contentWindowProxy = iframeProxy['contentWindow']; if (contentWindowProxy == null) { ... === |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
