Edgewall Software
Modify

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#10674 closed defect (fixed)

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

Reported by: Ryan J Ollos Owned by: Jun Omae
Priority: normal Milestone: 1.0.3
Component: wiki system Version: 0.13dev
Severity: normal Keywords:
Cc: Jun Omae
Release Notes:

Make [[Workflow]] macro work on auto-preview and prevent reflow when rendering workflow graph.

API Changes:

Added $.loadScript to avoid reloading scripts and $.documentReady to register ready listener. If the given script to $.loadScript is already loaded, instead call the registered ready listeners.

Description (last modified by Ryan J Ollos <ryano@…>)

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.

Attachments (2)

comment_10_ticket_7871.png (22.2 KB ) - added by Ryan J Ollos <ryano@…> 7 years ago.
t10674-overlap.png (66.9 KB ) - added by Christian Boos 4 years ago.
overlap issue with [b885614b03d3/jomae.git]

Download all attachments as: .zip

Change History (44)

Changed 7 years ago by Ryan J Ollos <ryano@…>

Attachment: comment_10_ticket_7871.png added

comment:1 Changed 7 years ago by Ryan J Ollos <ryano@…>

Description: modified (diff)

comment:2 Changed 7 years ago by Remy Blank

Milestone: next-major-0.1X

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.

comment:3 in reply to:  2 ; Changed 7 years ago by 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 …)

comment:4 Changed 7 years ago by 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.

comment:5 in reply to:  3 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by Jun Omae

Cc: Jun Omae added

comment:7 Changed 7 years ago by framay <franz.mayer@…>

Cc: franz.mayer@… added

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).

comment:8 in reply to:  7 Changed 7 years ago by 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.

comment:9 Changed 4 years ago by Ryan J Ollos

Reporter: changed from Ryan J Ollos <ryano@…> to Ryan J Ollos

comment:10 Changed 4 years ago by 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.

comment:11 Changed 4 years ago by Jun Omae

Milestone: next-major-releases1.0.3
Owner: set to Jun Omae
Status: newassigned

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

comment:12 Changed 4 years ago by 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).

comment:13 Changed 4 years ago by 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")

comment:14 Changed 4 years ago by 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.

comment:15 in reply to:  14 ; Changed 4 years ago by 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.

comment:16 in reply to:  15 Changed 4 years ago by 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.

comment:17 Changed 4 years ago by Jun Omae

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

Changed 4 years ago by Christian Boos

Attachment: t10674-overlap.png added

overlap issue with [b885614b03d3/jomae.git]

comment:18 in reply to:  17 ; Changed 4 years ago by 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:

overlap issue with [b885614b03d3/jomae.git]

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…

comment:19 in reply to:  18 Changed 4 years ago by 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

    diff --git a/trac/htdocs/js/workflow_graph.js b/trac/htdocs/js/workflow_graph.js
    index 4b14ce2..281edef 100644
    a b  
    223223                    source: nodes[sourceindex], target: nodes[targetindex]});
    224224      }
    225225
    226       var canvas = $('<canvas>').css({width: width, height: height})[0];
     226      var canvas = $('<canvas>').css({width: width, height: height,
     227                                      display: 'block'})[0];
    227228      canvas.width = $(canvas).width();
    228229      canvas.height = $(canvas).height();
    229230      if (typeof(G_vmlCanvasManager) != 'undefined')
  • trac/ticket/default_workflow.py

    diff --git a/trac/ticket/default_workflow.py b/trac/ticket/default_workflow.py
    index 9340278..b2b06e0 100644
    a b class WorkflowMacro(WikiMacroBase):  
    500500        return tag(
    501501            tag.div('', class_='trac-workflow-graph trac-noscript',
    502502                    id='trac-workflow-graph-%s' % graph_id,
    503                     style="height:%spx" % height),
     503                    style="display:inline-block;height:%spx" % height),
    504504            tag.noscript(
    505505                tag.div(_("Enable JavaScript to display the workflow graph."),
    506506                        class_='system-message')))

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

comment:20 Changed 4 years ago by 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.

comment:21 in reply to:  20 Changed 4 years ago by 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!

comment:22 Changed 4 years ago by Jun Omae

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

comment:23 Changed 4 years ago by Jun Omae

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:24 Changed 4 years ago by Jun Omae

Resolution: fixed
Status: closedreopened

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….

comment:25 in reply to:  24 Changed 4 years ago by 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)

comment:26 Changed 4 years ago by 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.

comment:27 Changed 4 years ago by 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

    diff --git a/trac/web/chrome.py b/trac/web/chrome.py
    index 5e70b8f..fc595df 100644
    a b class Chrome(Component):  
    10911091            if not int(req.session.get('accesskeys', 0)):
    10921092                stream |= self._strip_accesskeys
    10931093
    1094         links = req.chrome.get('links')
    1095         scripts = req.chrome.get('scripts')
    1096         script_data = req.chrome.get('script_data')
     1094        saved_chrome_data = dict((name, req.chrome.get(name))
     1095                                 for name in ('links', 'linkset', 'scripts',
     1096                                              'scriptset', 'script_data'))
    10971097        req.chrome['links'] = {}
     1098        req.chrome['linkset'] = set()
    10981099        req.chrome['scripts'] = []
     1100        req.chrome['scriptset'] = set()
    10991101        req.chrome['script_data'] = {}
    11001102        data.setdefault('chrome', {}).update({
    11011103            'late_links': req.chrome['links'],
    class Chrome(Component):  
    11111113                                               _invalid_control_chars)
    11121114        except Exception as e:
    11131115            # restore what may be needed by the error template
    1114             req.chrome['links'] = links
    1115             req.chrome['scripts'] = scripts
    1116             req.chrome['script_data'] = script_data
     1116            req.chrome.update(saved_chrome_data)
    11171117            # give some hints when hitting a Genshi unicode error
    11181118            if isinstance(e, UnicodeError):
    11191119                pos = self._stream_location(stream)

comment:28 in reply to:  27 Changed 4 years ago by 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!

comment:29 Changed 4 years ago by 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.

comment:30 Changed 4 years ago by 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.

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.

comment:31 Changed 4 years ago by 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.

comment:32 Changed 4 years ago by 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.

comment:33 in reply to:  32 ; Changed 4 years ago by 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?

comment:34 Changed 4 years ago by Franz Mayer <franz.mayer@…>

Cc: franz.mayer@… removed

comment:35 in reply to:  33 Changed 4 years ago by 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.

comment:36 in reply to:  33 ; Changed 4 years ago by 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.

comment:37 in reply to:  36 Changed 4 years ago by 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.

comment:38 Changed 4 years ago by Jun Omae

API Changes: modified (diff)
Resolution: fixed
Status: reopenedclosed

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

comment:39 Changed 4 years ago by 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.

Last edited 4 years ago by Peter Suter (previous) (diff)

comment:40 Changed 4 years ago by Peter Suter

Making $.loadScript not async seems to help:

  • trac/htdocs/js/trac.js

    diff -r d95b10358585 trac/htdocs/js/trac.js
    a b  
    142142      // "_=<timestamp>" parameter to url.
    143143      script = document.createElement("script");
    144144      script.src = href;
     145      script.async = false;
    145146      script.type = type || "text/javascript";
    146147      script.charset = charset || "utf-8";
    147148      $("head")[0].appendChild(script);

comment:41 Changed 4 years ago by 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.

comment:42 Changed 4 years ago by Peter Suter

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.