WorkFlow Macro displays "Enable JavaScript to display the workflow graph."

[Ryan J Ollos]

To reproduce:

• Go to ticket #7871.
• Add the text comment:10 to Add a comment.
• Wait for auto-preview to generate a preview.
• Observe that Enable JavaScript to display the workflow graph. is displayed in comment:10.

Observed on t.e.o with Windows 7 and Firefox 12.0.

[Remy Blank]

That's because we render the workflow graph only once at page render time. We currently don't have a mechanism to call processor- or macro-provided JavaScript at preview time. The same will probably happen in the automatic wiki preview.

[Ryan J Ollos <ryano@…>]

Replying to rblank:

That's because we render the workflow graph only once at page render time. We currently don't have a mechanism to call processor- or macro-provided JavaScript at preview time. The same will probably happen in the automatic wiki preview.

I've also noticed in 0.12.3 that pygments syntax highlighting is not present when auto-previewing a ticket comment. Syntax highlighting is present after one explicitly invokes a preview. Is that possibly for the same reason? (although I thought Pygments was a pure-Python package, so maybe not …)

[Remy Blank]

It's not exactly the same reason, but it's related. Syntax highlighting processors provide the CSS to be used after page rendering has already begun, so we can't add it as a <link> in the page header. Instead, it's loaded with JavaScript at the end of rendering. If you happen to already have a syntax highlighting processor on the ticket when you start adding a comment, rendering works fine, because the CSS is already loaded. Try adding a comment with a {{{#!python}}} block to e.g. #10644 if you want to see it in action.

[Jun Omae]

Replying to Ryan J Ollos <ryano@…>:

I've also noticed in 0.12.3 that pygments syntax highlighting is not present when auto-previewing a ticket comment.

This issue has been reported as #10470.

[framay <franz.mayer@…>]

When using Workflow macro in Wiki page, it always displays message Enable JavaScript to display the workflow graph. See ​demo-0.13/SandBoxWorkflowTest.

Tested with Firefox 12.0, Chrome 18.0 and IE 9.0 (all under Win7).

[Christian Boos]

Replying to framay <franz.mayer@…>:

When using Workflow macro in Wiki page, it always displays message Enable JavaScript to display the workflow graph. See ​demo-0.13/SandBoxWorkflowTest.

Tested with Firefox 12.0, Chrome 18.0 and IE 9.0 (all under Win7).

Thanks for the report, but that problem was already fixed in r11033 and the 0.13-demo site was not yet updated.

[Jun Omae]

Proof of concept can be found in jomae.git@t10674_1.0. After the patch, script data and script elements which are added by macros while rendering wiki append to response of XMLHttpResponse.

[Jun Omae]

Updated jomae.git@t10674_1.0, targeting 1.0.3. Rather, should we retarget 1.1.3? Also, could someone please review it?

[Ryan J Ollos]

The changes work well for me. I'll look at the code more closely tomorrow. I think it's good to target to 1.0.3 since it's a pretty significant defect in Trac. I can't see how it would negatively affect any plugins, do you? (On the other hand I'm reconsidering for some of the changes I've pushed to #11813 recently, which probably should have been targeted to the trunk because they are aesthetic issues only, and changing the structure of the page can negatively affect plugins).

[Christian Boos]

Reviewing:

• for [95943ba069/jomae.git] commit message, it feels like "prevent reflow by rendering workflow graph" is wrongly worded:
  • either "prevent reflow when rendering workflow graph"
  • or "prevent reflow by pre-setting placeholder size before rendering workflow graph"
• for the change itself, it works but on Firefox I see that the system-message box is visible for quite some time before it gets replaced with the workflow graph; looking again, it's also there flickering in Chrome. I thought about adding a visibility:hidden rule on it with Javascript? → log:cboos.git@review/t10674_1.0
  With that the "red box" flicker is no longer there, but it seems that Firefox collapses the space allocated to the box even though I think it shouldn't. The only browser in which this seems to work flawlessly is IE, probably not a good sign ;-) Maybe you can improve upon my experiment?
• for [732cc77619/jomae.git], looks good! (nitpick for the commit message, "make … macro work on auto-preview")

[Jun Omae]

Thanks for the review! Reviewed commit messages will be used when I'll push the changes.

Another solution for the red box flicker can be found in [3adeb8dd/jomae.git]. noscript element is used in the changes.

[Christian Boos]

Replying to jomae:

Another solution for the red box flicker can be found in [3adeb8dd/jomae.git]. noscript element is used in the changes.

Yes, nicer solution than mine! The red flicker is completely gone, but there's still some collapse/expand that can be seen with FF (33.1) and Chrome (38.0.2125.111). Only IE 11 seems able to avoid the reflow.

[Jun Omae]

Replying to cboos:

Replying to jomae:

Another solution for the red box flicker can be found in [3adeb8dd/jomae.git]. noscript element is used in the changes.

Yes, nicer solution than mine! The red flicker is completely gone, but there's still some collapse/expand that can be seen with FF (33.1) and Chrome (38.0.2125.111). Only IE 11 seems able to avoid the reflow.

Hmm, I cannot still reproduce the collapse/expand problems.

Also, functional tests fail with that changes. XHTML1 strict DTD disallows the noscript element in head element. I'll rework it.

[Jun Omae]

Updated jomae.git@t10674_1.0. Could you please try it?

[Christian Boos]

overlap issue with [b885614b03d3/jomae.git]

[Christian Boos]

Replying to jomae:

Updated jomae.git@t10674_1.0. Could you please try it?

Nearly there! The collapse/expand issue is indeed gone (in FF, Chrome, still fine in IE) but there's now another glitch: in those 3 browsers, there's an unfortunate interaction when a float is around, like in the TracWorkflow page:

Looking for ways to prevent it, so far no luck except for a { display: inline !important } or (inline-block) which only works in FF and doesn't sound right anyway…

[Christian Boos]

Replying to cboos:

Looking for ways to prevent it, so far no luck…

Ok, with this it seems to work:

trac/htdocs/js/workflow_graph.js

@@ -223 +223 @@
                     source: nodes[sourceindex], target: nodes[targetindex]});
       }
 
-      var canvas = $('<canvas>').css({width: width, height: height})[0];
+      var canvas = $('<canvas>').css({width: width, height: height,
+                                      display: 'block'})[0];
       canvas.width = $(canvas).width();
       canvas.height = $(canvas).height();
       if (typeof(G_vmlCanvasManager) != 'undefined')

trac/ticket/default_workflow.py

@@ -500 +500 @@
         return tag(
             tag.div('', class_='trac-workflow-graph trac-noscript',
                     id='trac-workflow-graph-%s' % graph_id,
-                    style="height:%spx" % height),
+                    style="display:inline-block;height:%spx" % height),
             tag.noscript(
                 tag.div(_("Enable JavaScript to display the workflow graph."),
                         class_='system-message')))

(inline would also do, but inline-block sounds better as we're dealing with a div…)

[Jun Omae]

I just created patch similar to that, [bcf71c284/jomae.git]. The canvas element is defined as ​block element, but browsers practically render as inline-block.

[Christian Boos]

Replying to jomae:

I just created patch similar to that, [bcf71c284/jomae.git].

Flawless behavior in all the browsers tested (FF, Chrome, IE on Windows). Great job!

[Jun Omae]

Okay. Thanks for your reviewing and testing! I'm going to push the changes and close #10674,​10470.

[Jun Omae]

Pushed in [13330-13331] and merged to trunk in [13332].

[Jun Omae]

This site is already deployed with Trac 1.1.3dev-r13332. But, the workflow macro doesn't work on auto-preview. I start to investigate the issue….

[Christian Boos]

Replying to jomae:

This site is already deployed with Trac 1.1.3dev-r13332. But, the workflow macro doesn't work on auto-preview. I start to investigate the issue….

Works for me… did you force reload the scripts? (cf. #9936)

[Jun Omae]

Yeah, I tried force reload. But it doesn't work. I just found how to reproduce.

Adding workflow macro to ticket comment, works fine. However, adding workflow macro to description in ticket box, the workflow macro renders blank.

[Jun Omae]

The following patch would fix it. I think we should clear linkset and scriptset of req.chrome in Chrome.render_template().

The issue occurs on only trunk, but I think 1.0-stable has the same issue and need apply it.

trac/web/chrome.py

@@ -1091 +1091 @@
             if not int(req.session.get('accesskeys', 0)):
                 stream |= self._strip_accesskeys
 
-        links = req.chrome.get('links')
-        scripts = req.chrome.get('scripts')
-        script_data = req.chrome.get('script_data')
+        saved_chrome_data = dict((name, req.chrome.get(name))
+                                 for name in ('links', 'linkset', 'scripts',
+                                              'scriptset', 'script_data'))
         req.chrome['links'] = {}
+        req.chrome['linkset'] = set()
         req.chrome['scripts'] = []
+        req.chrome['scriptset'] = set()
         req.chrome['script_data'] = {}
         data.setdefault('chrome', {}).update({
             'late_links': req.chrome['links'],
@@ -1111 +1113 @@
                                                _invalid_control_chars)
         except Exception as e:
             # restore what may be needed by the error template
-            req.chrome['links'] = links
-            req.chrome['scripts'] = scripts
-            req.chrome['script_data'] = script_data
+            req.chrome.update(saved_chrome_data)
             # give some hints when hitting a Genshi unicode error
             if isinstance(e, UnicodeError):
                 pos = self._stream_location(stream)

[Christian Boos]

Replying to jomae:

The following patch would fix it. I think we should clear linkset and scriptset of req.chrome in Chrome.render_template().

The issue occurs on only trunk, but I think 1.0-stable has the same issue and need apply it.

Patch deployed on demo-1.1, seems to work just fine!

[Jun Omae]

I misunderstood it. Also, reproduced with 1.0-stable.

1. add textarea with wiki format to [ticket-custom] in trac.ini.
2. write {{{#!ini ... }}} or workflow macro to the field.
3. auto preview doesn't add pygments.css and workflow_graph.js.

The issue is caused by calling format_to_html from _prepare_fields() at branches/1.0-stable/trac/ticket/web_ui.py@13344:1552#L1549​. In trunk, the format_to_html is called for also ticket's description.

Please revert the patch. I'll rework it.

[Jun Omae]

The following changes move rendering using format_to_* for custom fields with wiki format to ticket_box.html from _prepare_fields(). It works fine to me.

• [84655193/jomae.git] for trunk
• [5dcdee60/jomae.git] for 1.0-stable

I noticed another issue. After [12244] in trunk, format_to_html is called twice for ticket's description. After the changes, the method would be called once.

[Christian Boos]

I was going to ask anyway for your previous solution, but now I really have to: can you please try to describe a bit what's going on, why we had the problem in the first place and if there could be guidelines for developers to follow so that the css and scripts will be taken into account? Or are we just not enough "robust" here, like, should we gather things from scripts and late scripts at once? Even the "late" scripts would benefit from being documented better with a few comments and/or doc string, I'm quite sure I don't recall the whole story…

Once I understand the issue enough, I'll try to expand the ApiDocs a bit in this direction.

[Jun Omae]

The issue in comment:24 is caused by the following conditions;

1. process_request() calls directly format_to_* methods. Scripts and css would be added to req.chrome.{scripts,links} while rendering wiki text.
2. process_request() returns template file which doesn't include layout.html, (e.g. ticket_box.html). In the case, the scripts and stylesheets in step 1 wouldn't be used because layout.html normally render them.
3. wiki_to_html is called in the template file while rendering. If the same scripts and css in step 1 are added, the scripts and css wouldn't be actually added because req.chrome.{linkset,scriptset} detects that they are already added.

So, if chrome_info_script is used in template and layout.html isn't included from the template (e.g. for ajax response), don't call format_to_* methods before rendering template. Instead, use wiki_to_* methods in the template.

Or are we just not enough "robust" here, like, should we gather things from scripts and late scripts at once?

I've tried it. But core scripts and css files, jquery.js, trac.js, babel.js, trac.css, etc., would be gathered by chrome_info_script. That's so bad.

[Christian Boos]

Replying to jomae:

The issue in comment:24 is caused by the following conditions;

Thanks a lot for your very clear explanations!

Or are we just not enough "robust" here, like, should we gather things from scripts and late scripts at once?

I've tried it. But core scripts and css files, jquery.js, trac.js, babel.js, trac.css, etc., would be gathered by chrome_info_script. That's so bad.

But couldn't we adopt the same strategy for <script> than what you added to loadStyleSheet() for avoiding the reload a stylesheet already present?

[Jun Omae]

Replying to cboos:

I've tried it. But core scripts and css files, jquery.js, trac.js, babel.js, trac.css, etc., would be gathered by chrome_info_script. That's so bad.

But couldn't we adopt the same strategy for <script> than what you added to loadStyleSheet() for avoiding the reload a stylesheet already present?

workflow_graph.js renders graph using $.ready(). Therefore, we must load the script and cannot adopt the same strategy for scripts….

And another issue, the core scripts and css wrongly are loaded by auto-preview on editting ticket commnet. I try to make robust.

[Jun Omae]

Replying to cboos:

But couldn't we adopt the same strategy for <script> than what you added to loadStyleSheet() for avoiding the reload a stylesheet already present?

jomae.git@t10674v3_1.0:

• added $.loadScript to avoid reloading scripts
• added $.documentReady which is to register ready listener. If the given script to $.loadScript is already loaded, instead call the listener

I confirmed with Firefox 33, Chrome 39, IE 11 and IE 8.

[Christian Boos]

Replying to jomae:

• added $.loadScript to avoid reloading scripts

Nice!

• added $.documentReady which is to register ready listener. If the given script to $.loadScript is already loaded, instead call the listener

What I had in mind was to dissociate the "library" aspect of a given .js script (which would be loaded by a <script src="…">) and its invocation with .ready(...) (in a <script>…</script> snippet), but your approach is more elegant.

[Jun Omae]

Thanks. Committed in [13389-13391] and merged to trunk in [13392]. Re-closing.

[Peter Suter]

After this change I have problems with macros that load e.g. jQuery UI:

from trac.web.chrome import Chrome
from trac.wiki.macros import WikiMacroBase

class TestMacro(WikiMacroBase):
    def expand_macro(self, formatter, name, content):
        chrome = Chrome(self.env)
        chrome.add_jquery_ui(formatter.req)
        return ''

About half the time I randomly get Javascript errors like $.datepicker is undefined in Firefox or Cannot read property 'setDefaults' of undefined in Chrome.

[Peter Suter]

Making $.loadScript not async seems to help:

trac/htdocs/js/trac.js

@@ -142 +142 @@
       // "_=<timestamp>" parameter to url.
       script = document.createElement("script");
       script.src = href;
+      script.async = false;
       script.type = type || "text/javascript";
       script.charset = charset || "utf-8";
       $("head")[0].appendChild(script);

[Jun Omae]

Oh. Good catching! You're right. I've thought that script.async is false by default. Please apply it.

See also: ​https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#Async_support.

[Peter Suter]

Thanks for confirming! Committed to 1.0-stable in [13493] and merged trunk in [13494].