Issue 2998: add meaningful css classes to new change screen
Status:  Released
Owner: ----
Closed:  Nov 2014
Cc:
Reported by sdague, Oct 30, 2014
As of Gerrit 2.8 and the new change screen many of the semanticly useful css classes have been stripped out of the UI. This makes site theming much more difficult, as well doing interesting things with the UI in javascript much harder.

For instance, in OpenStack gerrit we have some custom js to provide CI system rollup - . 

In the old change screen we can select and parse by css classes. For instance:

var ci_parse_comments = function() {
    var comments = [];
    $(".commentPanel").each(function() {
        var comment = {};
        comment.name = $(this).attr("name");
        comment.email = $(this).attr("email");
        comment.date = $(this).find(".commentPanelDateCell").attr("title");
        var comment_panel = $(this).find(".commentPanelMessage");
        comment.psnum = ci_parse_psnum(comment_panel);
        comment.merge_conflict = ci_parse_is_merge_conflict(comment_panel);
        comment.pipeline = ci_find_pipeline(comment_panel);
        comment.results = ci_parse_results(comment_panel);
        comment.is_ci = (ciRegex.exec(comment.name) !== null);
        comment.is_trusted_ci = (trustedCIRegex.exec(comment.name) !== null);
        comment.ref = this;
        comments.push(comment);
    });
    return comments;
};

However in the new screen, none of the .comment* classes exist any more, and neither do any of the attributes that were set int he markup.

We instance have to play a game of selecting on <p> tags, and playing positional guessing games to find the right fields. The equivalent function that supports the new screen is.

var ci_parse_comments = function() {
    var comments = [];
    $("p").each(function() {
        var match = psRegex.exec($(this).html());
        if (match !== null) {
            var psnum = parseInt(match[2]);
            var top = $(this).parent().parent().parent();
            // old change screen
            var name = top.attr("name");
            if (!name) {
                // new change screen
                name = $(this).parent().prev().children()[0].innerHTML;
            }
            var comment = {};
            comment.name = name;

            var date_cell = top.find(".commentPanelDateCell");
            if (date_cell.attr("title")) {
                // old change screen
                comment.date = date_cell.attr("title");
            } else {
                // new change screen
                comment.date = $(this).parent().prev().children()[2].innerHTML;
            }
            var comment_panel = $(this).parent();
            comment.psnum = psnum;
            comment.merge_conflict = ci_parse_is_merge_conflict(comment_panel);
            comment.pipeline = ci_find_pipeline(comment_panel);
            comment.results = ci_parse_results(comment_panel);
            comment.is_ci = (ciRegex.exec(comment.name) !== null);
            comment.is_trusted_ci = (trustedCIRegex.exec(comment.name) !== null);
            comment.ref = top;
            comments.push(comment);
        }
    });
    return comments;
};

Note the use of having to do things like $(this).parent().prev().children()[2].innerHTML to find data. This is extremely fragile and sensitive to future breakage because there is nothing semantically meaningful we are able to grab on. 

It also means that if we want to do something like change css attributes on the date field for display... it's not very cleanly possible (again, there are positional css hacks to do it, but pretty fragile).

Old screen's semantic tagging of classes was great for extending gerrit. Please bring it back into the new change screen for both theming and client side extending.
Nov 12, 2014
Project Member #1 zaro0508
David P.  Do you know if this issue has been fixed upstream?  
Cc: david.pu...@sonymobile.com
Nov 12, 2014
#2 sdague
It appears that at least on gerrit-review.g.c today for the div tags there are now full java class tags.

<td class="com-google-gerrit-client-change-ChangeScreen2_BinderImpl_GenCss_style-labelName com-google-gerrit-client-change-ChangeScreen2_BinderImpl_GenCss_style-label_need">Code-Review</td>

However we are still missing semantic tags on the +1 / -1 votes for instance. 

<span title="Looks good to me, but someone else must approve">+1 <span role="listitem" data-id="1012987" title="hugo.ares@ericsson.com" class="com-google-gerrit-client-change-ChangeScreen2_BinderImpl_GenCss_style-label_user"><img class="com-google-gerrit-client-change-ChangeScreen2_BinderImpl_GenCss_style-avatar" src="https://lh3.googleusercontent.com/-XdUIqdMkCWA/AAAAAAAAAAI/AAAAAAAAAAA/4252rscbv5M/s26/photo.jpg" height="26">Hugo Arès</span></span>


Nov 12, 2014
Project Member #3 david.pu...@sonymobile.com
This migth have been fixed by [1] which is in 2.9

[1] https://gerrit-review.googlesource.com/#/c/53593/

Nov 12, 2014
#4 sdague
@david is there a reason that it wasn't set to "pretty" instead of "stable"? 
Nov 12, 2014
Project Member #5 david.pu...@sonymobile.com
Sorry, I have no idea.  It would be better to post your question as a comment on that change, or upload a new commit that changes "stable" to "pretty".

Nov 19, 2014
Project Member #6 david.pu...@sonymobile.com
Changing it to "pretty" doesn't help.  The problem seems to be that the DIV element that holds the comment does not have any styleName attribute in the XML file.

With [1] the DIV now has:

  stylename="com-google-gerrit-client-change-Message_BinderImpl_GenCss_style-comment"

[1] https://gerrit-review.googlesource.com/61744
Status: ChangeUnderReview
Nov 20, 2014
Project Member #7 david.pu...@sonymobile.com
(No comment was entered for this change.)
Status: Submitted
Labels: FixedIn-2.9.2
Nov 30, 2014
Project Member #8 david.pu...@sonymobile.com
(No comment was entered for this change.)
Status: Released