1059723 - Reply button should become AJAX-based

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Koosha KM]

After markdown integration, the reply button on the comment box should become AJAX-based because the current one would destroy the HTML content of a comment if the comment has markdown structures. So, the reply button should load the raw markdown-based comment from DB via an AJAX call if the comment is markdown-based. The behavior of the button could be left intact for non-markdown comments.

Comment 1

[:glob ✱]

adding comments without a page refresh shouldn't be linked with the format of the comment; we should either always do it, or not do it at all.

this is a complicated issue to resolve because we need to consider mid-air collisions with other comments/changes.

(i'm pretty sure this is a duplicate, but i can't find one right now).

Comment 2

[:glob ✱]

oh, wait, i totally miss-read this bug; sorry about that koosha (drinks more morning coffee).

this should block 5.0.

Comment 3

[Dave Miller [:justdave]]

To be clear in case anyone else had the same confusion glob and I initially did, this is for loading the existing comment to be quoted and placed into the comment edit box for the new comment, not for submitting the new comment.

Comment 4

[Koosha KM]

Attachment: patch-v1.0

Comment 5

[:glob ✱]

Comment on attachment 8484921 [details] [diff] [review]
patch-v1.0

as per irc, reassigning the review to sgreen.

from a quick look at the code..
> sub _DoBlockQuotes
it would probably be better to pass in an option to the code which adds the quote spans to skip them, instead of adding them and then removing them later.

Comment 6

[Koosha KM]

Attachment: patch-v2.0

I added the code to skip the quote coloring in quoteUrls().

Comment 7

[:glob ✱]

Comment on attachment 8485626 [details] [diff] [review]
patch-v2.0

Review of attachment 8485626 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Markdown.pm
@@ +414,5 @@
> +            # These leading spaces screw with <pre> content, so we need to fix that:
> +            $bq =~ s{(\s*<pre>.+?</pre>)}{
> +                        my $pre = $1;
> +                        $pre =~ s/^  //mg;
> +                        $pre;

we're switch from <pre> tags in bug 1059684, so this will need to be updated.

::: Bugzilla/Template.pm
@@ +225,5 @@
> +    unless (scalar caller eq 'Bugzilla::Markdown') {
> +        # Color quoted text
> +        $text =~ s~^(&gt;.+)$~<span class="quote">$1</span >~mg;
> +        $text =~ s~</span >\n<span class="quote">~\n~g;
> +    }

using the call stack to control behaviour isn't the best approach here - it's prone to breaking, and means this behaviour can't be used by other code.

add an optional parameter to quoteUrls() instead.

::: skins/standard/global.css
@@ +250,5 @@
> +        padding: 0;
> +        margin-left: 0.5em;
> +        margin-bottom: -1em;
> +        white-space: pre;
> +    }

the white-space:pre will also need to be updated to accommodate changes in bug 1059684.

Comment 8

[Koosha KM]

(In reply to Byron Jones ‹:glob› from comment #7)
> Comment on attachment 8485626 [details] [diff] [review]
> patch-v2.0
> 
> Review of attachment 8485626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: Bugzilla/Markdown.pm
> @@ +414,5 @@
> > +            # These leading spaces screw with <pre> content, so we need to fix that:
> > +            $bq =~ s{(\s*<pre>.+?</pre>)}{
> > +                        my $pre = $1;
> > +                        $pre =~ s/^  //mg;
> > +                        $pre;
> 
> we're switch from <pre> tags in bug 1059684, so this will need to be updated.

The code comes from the original implementation and I don't think that it has anything to do with the way Bugzilla shows comments. Code blocks do have <pre> and this piece of code deals with them.

Comment 9

[Koosha KM]

Attachment: patch-v3

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8495119 [details] [diff] [review]
patch-v3

Will review this tomorrow.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8495119 [details] [diff] [review]
patch-v3

Review of attachment 8495119 [details] [diff] [review]:
-----------------------------------------------------------------

These can be fixed on commit. r=dkl

::: skins/standard/global.css
@@ +249,5 @@
> +        color: #65379C;
> +        padding: 0;
> +        margin-left: 0.5em;
> +        margin-bottom: -1em;
> +        white-space: pre;

Remove the white-space line and add:

padding-left: 0.5em;

::: template/en/default/bug/comments.html.tmpl
@@ +14,4 @@
>  <script type="text/javascript">
>  <!--
>    /* Adds the reply text to the 'comment' textarea */
> +  function replyToComment(id, real_id, name, comment_text) {

s/comment_text/text/

@@ +25,5 @@
> +            var text_elem = document.getElementById('comment_text_'+id);
> +            text = getText(text_elem);
> +        } else {
> +            text = comment_text;
> +        }

Simplify:

if (text == null) {
    var text_elem = document.getElementById('comment_text_'+id);
    text = getText(text_elem);
}

Comment 12

[Simon Green]

Given the number of changes required "on commit", I would prefer to see a new patch being reviewed before it is committed.

Comment 13

[Koosha KM]

(In reply to David Lawrence [:dkl] from comment #11)
> ::: skins/standard/global.css
> @@ +249,5 @@
> > +        color: #65379C;
> > +        padding: 0;
> > +        margin-left: 0.5em;
> > +        margin-bottom: -1em;
> > +        white-space: pre;
> 
> Remove the white-space line and add:
> 
> padding-left: 0.5em;
> 

The white-space line is crucial to making lines wrap correctly. Try nested quoted lines with and without it, you will see the difference.

Comment 14

[Koosha KM]

Attachment: patch-v4

New patch with the requested changes.

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 8504707 [details] [diff] [review]
patch-v4

r=dkl

Comment 16

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   847191a..42d5e45  master -> master