Ticket validation warnings are difficult to notice

[thomas.akesson@…]

When a ticket validation fails, e.g. due to ITicketManipulator.validate_ticket(..), a warning message is displayed at the top of the page (just below nav). However, the page is scrolled to #trac-add-comment, which means the warning is most likely not in view. Inexperienced users might not notice that the change/comment was not saved.

When the validation fails due to a concurrent change, there are 2 informations displayed:

• The warning at the top of the page
• The explanation near the comment field

This makes it much easier to notice the issue. It would make sense to display other validation warnings also near the comment field.

Is there support for providing both a warning and an explanation message from ITicketManipulator.validate_ticket(..) or is there some other mechanism used when a concurrent change is detected?

I can probably produce a patch with some guidance.

[Ryan J Ollos]

I think I have a note somewhere to remind myself to try and fix this, but I'd be happy if you produced a patch :) The necessary change might be similar to #11102.

There is also a similar issue on the Plugin admin panel. After enabling or disabling a plugin, there is a notice displayed in the standard location below the contextual navigation, but the page scrolls to the plugin for which the changes were just applied, so the notice is almost never visible (​th:comment:25:ticket:10741).

[Ryan J Ollos]

I'm not sure if this will make it into 1.0.2, but I plan to do something about it.

[thomas.akesson@…]

I did some investigation on how the warnings are produced.

• The ticket validation warnings are produced using add_warning on the req. Displayed by theme.html.
• The concurrent change warning seems to have special support in the ticket template.

Some ideas on different approaches:

• Duplicate the warning block in theme.html to make the warnings appear twice: current location (top) and just above the footer.
• Introduce display of warnings in ticket.html, e.g. just above the concurrent edit warning ("trac-edit-warning").
• Is there some way of avoiding the scrolling to force the window position to stay at the top when there is a warning? Anything useful in #11084 ?

[Ryan J Ollos]

Replying to thomas.akesson@…:

• Introduce display of warnings in ticket.html, e.g. just above the concurrent edit warning ("trac-edit-warning").
• Is there some way of avoiding the scrolling to force the window position to stay at the top when there is a warning? Anything useful in #11084 ?

Your analysis looks good. I think either of these two options would work well, however there are complications that I'll mention in a moment.

I tried the following based on your suggestion,

trac/ticket/templates/ticket.html

@@ -182 +182 @@
         <div py:if="ticket.exists and can_append" id="trac-add-comment" class="
           <h3 class="foldable" id="edit">Add Comment</h3>
           <div>
+            <div id="warning" py:if="chrome.warnings" class="system-message">
+              <py:choose test="len(chrome.warnings)">
+                <strong>Warning:</strong>
+                <py:when test="1">${chrome.warnings[0]}</py:when>
+                <py:otherwise><ul><li py:for="warning in chrome.warnings">$warn
+              </py:choose>
+            </div>
             <div id="trac-edit-warning" class="warning system-message"
                  style="${'display: none' if start_time == ticket['changetime']
                  i18n:msg="">

One easy way to produce a warning is to remove the Summary from a ticket. Here is the case of a ticket change in which the summary was removed, and there is also a concurrent edit collision:

We are displaying warnings in the ticket warning box that have nothing to do with the ticket. Maybe if add_warning had a realm parameter, we could filter the warnings to just the ticket realm. Alternatively, we could wrap the add_warning calls in tags/trac-1.0.1/trac/ticket/web_ui.py​ with a function that also passes the text to the template to be displayed in our warning box above the ticket comment box. I tend to think the latter could work okay. In that case, we could also not show the Sorry you cannot save your changes… here, and just show the This ticket has been modified … in the Warning: box.

Since you mentioned #11084, I started looking at the scrollToTop​ function. However, from what I see this code can be removed since it only appears when the user presses Preview, and when a preview is requested the scroll is handled by POSTing to the href #trac-add-comment: tags/trac-1.0.1/trac/ticket/templates/ticket.html@:182#L180​. The scrollToTop was added in [8753#file8]. It may have been intended to work in auto-preview mode, but if that is the case we'll have to add logic in the JavaScript code rather than the template code.

For the case of the New Ticket page, a similar thing happens when you try to create a ticket without a Summary. The warning can't be seen on the page because the #ticket fragment in the URL causes a scroll as well: tags/trac-1.0.1/trac/ticket/templates/ticket.html@:183#L180​.

In order to prevent the scrolling, it appears we'd need to somehow remove or not add the fragment when warnings are present.

[ImPortugueseButNotRonaldo]

Hello, I solved this specifically with a solution in the net about javascript scrollable panes and applied it to the ticket warning box, here are the steps to change in theme.html:

1. Create a father div for the warning box div and give it the style 1margin-top:10px; width: 100%; z-index: 9991
2. Create a div before the one in 1) that will serve as an anchor
3. Some javascript text before all of these div's and some javascript after

There is a small flicker problem but its acceptable (I think). Here is the full code:

<script type='text/javascript'>//<![CDATA[
function moveScroller() {
    var move = function() {
        var st = $(window).scrollTop();
        var ot = $("#scroller-anchor").offset().top;
        var s = $("#scroller");
        if(st > ot) {
            s.css({
                position: "fixed",
                top: "0px"
            });
        } else {
            if(st <= ot) {
                s.css({
                    position: "relative",
                    top: ""
                });
            }
        }
    };
    $(window).scroll(move);
    move();
}//]]>
</script>
      <div id="scroller-anchor"></div>
      <div id="scroller" style="margin-top:10px;width: 100%;z-index: 999">
      <div id="warning" py:if="chrome.warnings" class="system-message">
        <a class="trac-close-msg" href="#" title="Hide this warning"><span>close</span></a>
        <py:choose test="len(chrome.warnings)">
          <strong>Atenção:</strong>
          <py:when test="1">${chrome.warnings[0]}</py:when>
          <py:otherwise><ul><li py:for="warning in chrome.warnings">$warning</li></ul></py:otherwise>
        </py:choose>
      </div>
      </div>
        <script type="text/javascript">
        $(function() {
                moveScroller();
        });
</script>

[Thomas Schiefer]

The javascript solution is working like a charm. Totally solved the problem. Thx!

[martin@…]

I see the JS workaround, and may use it. But is this still going to get fixed in trac core, so we don't have to workaround? Not trying to hurry, just checking it was still planned, and registering my interest.

[martin@…]

I took the solution from comment:6 and turned it into a pure JS solution. Now you don't have to mess around with theme.html - simply include the attached javascript file and it does the rest.

[Ryan J Ollos]

Another case I encountered today in which the warning is not visible is when the comment size exceeds max_comment_size. The mechanism is the same as what is discussed in this ticket, just a different trigger.

[Ryan J Ollos]

Moved ticket assigned to next-stable-1.0.x since maintenance of 1.0.x is coming to a close. Please move the ticket back if it's critical to fix on 1.0.x.

[Ryan J Ollos]

Progress on the issue is being made in #12725 (changes targeted to 1.3.x). If those changes are committed, this ticket could still address:

• Visibility of notices on plugin admin page (comment:1)
• Display of general warnings in the ticket validation warning area (comment:4): replace the add_warning calls in _validate_ticket​​ with a function that also creates a list of warnings to be displayed in our warning box above the ticket preview.
• Possibly address this issue on 1.2-stable. An alternate approach for 1.2-stable might be to not scroll to the preview when there is a warning displayed.

[c.feige@…]

Hello

A lot of discussion here is about ticket warnings when applying ticket comments. However, please consider also other ticket edits for example collisions of the ticket description body. This is (another) reason for why it may not be desirable to display the ticket warning above the "ticket comment" area.

My recommendation is to put such warnings near the "SUBMIT" button. Because we want to warn people before submitting ticket changes (not only comments) which provoke a collision. In other words: Hitting the "SUBMIT" button makes the actual ticket changes and collision, this is why we need the warning there and not on other places.

Clemens

[anonymous]

I am still using TRAC 1.0 but noticed that in TRAC 1.2 the "COMMENT" field has moved below the "EDIT" field. This is a very good decision because then the "COMMENT" field (together with any WARNING boxes…) moves closes to the "SUBMIT" button.

TRAC 1.0	TRAC 1.2
attachments change history add comments (+ possible WARNING box) modify ticket SUBMIT button	attachments change history modify ticket add comments (+ possible WARNING box) SUBMIT button

Replying to c.feige@…:

My recommendation is to put such warnings near the "SUBMIT" button.

[Ryan J Ollos]

I'd like to improve the behavior for the ticket page on 1.2-stable, without putting too much effort into it, and then create a new ticket for 1.4-stable if there are any remaining issues.

I'm considering the following to prevent scrolling when there is a warning. Thoughts?

trac/ticket/templates/ticket.html

@@ -28 +28 @@
       jQuery(document).ready(function($) {
         $("div.description").find("h1,h2,h3,h4,h5,h6").addAnchor(_("Link to this section"));
         $(".foldable").enableFolding(false, true);
+      <py:if test="req.chrome['warnings']">
+        location.hash = '';
+      </py:if>
       <py:when test="ticket.exists">/*<![CDATA[*/
         $("#attachments").toggleClass("collapsed");
         $("#trac-up-attachments").click(function () {
@@ -323 +326 @@
 
         <!--! Add comment -->
         <div py:if="ticket.exists and can_append" id="trac-add-comment"
-             class="${classes('field', 'trac-scroll' if preview_mode else None)}">
+             class="${classes('field', 'trac-scroll' if preview_mode and
+                                                     not req.chrome['warnings']
+                                                     else None)}">
           <h3 class="foldable" id="edit">Add Comment</h3>
           <div>
             <div id="trac-edit-warning" class="warning system-message"

The downside is that the warning could be unrelated to the ticket, like Can't synchronize with repository. However, I think it's still an improvement. Also, we should consider whether Can't synchronize with repository should be displayed as a warning. It's a configuration issue and we should consider if only TRAC_ADMIN should see it, or if it should be logged rather than displayed on the page.

[Ryan J Ollos]

The comment:16 change seems ugly. Will check if the issue still exists on 1.4-stable.

[Ryan J Ollos]

Replying to Ryan J Ollos:

We are displaying warnings in the ticket warning box that have nothing to do with the ticket.

I've been considering we should either remove these "Can't synchronize" warnings or only show them if the user has TRAC_ADMIN (noted also in comment:16): #13301.

[Ryan J Ollos]

This was fixed in 1.4. See comment:description:ticket:13260 for a list of template locations the warnings are added.

[Ryan J Ollos]

Replying to Ryan J Ollos:

• Visibility of notices on plugin admin page (comment:1)

⇒ #13302.

[Ryan J Ollos]

Replying to Ryan J Ollos:

Since you mentioned #11084, I started looking at the scrollToTop​ function. However, from what I see this code can be removed since it only appears when the user presses Preview, and when a preview is requested the scroll is handled by POSTing to the href #trac-add-comment: tags/trac-1.0.1/trac/ticket/templates/ticket.html@:182#L180​. The scrollToTop was added in [8753#file8]. It may have been intended to work in auto-preview mode, but if that is the case we'll have to add logic in the JavaScript code rather than the template code.

I will do some more testing, but looks like we can remove trac-scroll class on 1.4-stable:

trac/ticket/templates/ticket.html

@@ -492 +492 @@
         ## Add comment
 
         # if ticket.exists and can_append:
-        <div id="trac-add-comment"
-             class="${classes('field', 'trac-scroll' if preview_mode)}">
+        <div id="trac-add-comment" class="field">
           <h3 class="foldable" id="edit">${_("Add Comment")}</h3>
           <div>

[Ryan J Ollos]

comment:21 patch applied in r17360.