Bug 130911 - Web Inspector: AXI: node-link-list should be collapsible
Summary: Web Inspector: AXI: node-link-list should be collapsible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Normal
Assignee: Aaron Chu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-28 13:00 PDT by James Craig
Modified: 2016-01-23 18:47 PST (History)
6 users (show)

See Also:


Attachments
screen shot (44.27 KB, image/png)
2014-03-28 13:00 PDT, James Craig
no flags Details
Patch (6.86 KB, patch)
2015-12-01 23:08 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (6.82 KB, patch)
2015-12-01 23:43 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (5.85 KB, patch)
2015-12-06 15:30 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (7.60 KB, patch)
2015-12-06 22:55 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2015-12-07 23:09 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2016-01-11 01:40 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (7.57 KB, patch)
2016-01-15 21:08 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2016-01-19 23:10 PST, Aaron Chu
no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2016-01-23 17:46 PST, Aaron Chu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2014-03-28 13:00:08 PDT
Web Inspector: AXI: node-link-list should be collapsible. Sometimes these lists get really long so it'd be good to include something like a disclosure button.
Comment 1 James Craig 2014-03-28 13:00:42 PDT
Created attachment 228079 [details]
screen shot
Comment 2 Radar WebKit Bug Importer 2014-03-28 13:01:12 PDT
<rdar://problem/16460250>
Comment 3 Timothy Hatcher 2014-03-28 16:46:55 PDT
This is making me wonder if Children is a good thing to include in the AX properties.
Comment 4 James Craig 2014-03-28 17:24:12 PDT
It's already very useful, and will be especially so with Chris' patch. Many times this list of accessibility siblings aren't even siblings. Sometimes they are promoted and normalized the in AX tree in a way that you can never see in the DOM.

It's only in a few cases (screen shot is on the body node) or in long lists (Facebook feed, Tweet list) where this would get unreasonably long. Perhaps we should make the Children list always collapsed by default, or collapsed if it's longer than n number of items.
Comment 5 James Craig 2014-03-28 17:25:23 PDT
(Chris's patch being bug 130563.)
Comment 6 James Craig 2014-03-28 17:27:38 PDT
W3C specs are another place where you can get giant lists of AX Children.
Comment 7 Timothy Hatcher 2014-03-28 17:38:30 PDT
I think making it collapsed with a "More…" inline link at the end would be a good start.
Comment 8 Joseph Pecoraro 2014-03-28 17:44:45 PDT
(In reply to comment #7)
> I think making it collapsed with a "More…" inline link at the end would be a good start.

I agree! And if we already know the #. "# More..."
Comment 9 James Craig 2014-03-28 18:30:01 PDT
How about the first 5 children (3?) plus the More link? Do we think we'll need a "Show less" link? They could always show less by just clicking away and back to the list, or if there is a "less" toggle, we could retain the current expanded/collapsed state for all nodes.
Comment 10 Timothy Hatcher 2014-03-28 18:43:37 PDT
5 sounds good. Just More… and clicking away to another DOM node seems fine.
Comment 11 Aaron Chu 2015-12-01 23:08:11 PST
Created attachment 266430 [details]
Patch
Comment 12 Aaron Chu 2015-12-01 23:43:25 PST
Created attachment 266432 [details]
Patch
Comment 13 James Craig 2015-12-02 03:05:31 PST
Comment on attachment 266432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266432&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:346
> +                    moreNodes.innerText = (listItemCount - initialItemCount) + " More\u2026";

Localize this string.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:347
> +                    moreNodes.setAttribute("role", "link");

Is there a reason you used span[role=link] instead of a[href]? I'd recommend a[href] because you get the focus and keypress events for free with your click handler. If you have a compelling reason to use the span[role=link], You should add [tabindex=0] and register the keypress handler for Return.

I haven't built this path yet, but on first glance, it appears the link would not be focusable. Although, not all of the Web Inspector is accessible yet, we should strive to avoid accessibility gotchas that will come back to bite us when trying to make the rest of WebInspectorUI accessible.
Comment 14 James Craig 2015-12-02 03:08:37 PST
Please forgive all the typos in my comments. It's 3am. Calling it a night. ;-)
Comment 15 Joseph Pecoraro 2015-12-02 13:31:24 PST
Comment on attachment 266432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266432&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:305
> +            var initialItemCount = 5;

You can make this a const variable with a more descriptive name:

    const itemsToShow = 5;

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:311
> +            var node;
> +            var link;
> +            var listitem;
> +            var nodeId;
> +            var moreNodes;

Style: Our style is to put the `var` (or more commonly now, `let`) next to where the variable is first used. Predefining these up at the top has no advantages and just causes visual noise.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:346
>> +                    moreNodes.innerText = (listItemCount - initialItemCount) + " More\u2026";
> 
> Localize this string.

Yep! Use WebInspector.UIString, then use `Tools/Scripts/update-webkit-localizable-strings` to automatically update the `Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js` list of localized strings.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:347
>> +                    moreNodes.setAttribute("role", "link");
> 
> Is there a reason you used span[role=link] instead of a[href]? I'd recommend a[href] because you get the focus and keypress events for free with your click handler. If you have a compelling reason to use the span[role=link], You should add [tabindex=0] and register the keypress handler for Return.
> 
> I haven't built this path yet, but on first glance, it appears the link would not be focusable. Although, not all of the Web Inspector is accessible yet, we should strive to avoid accessibility gotchas that will come back to bite us when trying to make the rest of WebInspectorUI accessible.

I agree, an <a href> would work well here.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:361
> +
> +

Unnecessary whitespace like this can be removed.

> ManualTests/accessibility/collapsible-node-link-list.html:2
> +<body id="body">

This id is probably not needed.
Comment 16 James Craig 2015-12-04 02:17:35 PST
Comment on attachment 266432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266432&action=review

>>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:347
>>> +                    moreNodes.setAttribute("role", "link");
>> 
>> Is there a reason you used span[role=link] instead of a[href]? I'd recommend a[href] because you get the focus and keypress events for free with your click handler. If you have a compelling reason to use the span[role=link], You should add [tabindex=0] and register the keypress handler for Return.
>> 
>> I haven't built this path yet, but on first glance, it appears the link would not be focusable. Although, not all of the Web Inspector is accessible yet, we should strive to avoid accessibility gotchas that will come back to bite us when trying to make the rest of WebInspectorUI accessible.
> 
> I agree, an <a href> would work well here.

Come to think of it, a styled <button> is probably even better since it also catches the spacebar click handler by default. In case you use <a href="" role="button", you should intercept spacebar clicks in JS.
Comment 17 Aaron Chu 2015-12-06 15:30:40 PST
Created attachment 266739 [details]
Patch
Comment 18 James Craig 2015-12-06 16:11:58 PST
Comment on attachment 266739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266739&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:305
>              var linkList = null;

You define linkList just inside the conditional and don't use it outside the conditional, so you can probably remove this one and move the var keyword to line 311 below.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:307
> +            var listItemCount = 0;
> +            var initiallyHiddenItems = [];

Ditto. If you don't need this outside the conditional, move the assignment inside.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:322
> +                                listitem.className += "hidden";

I think the HTML5 @hidden DOM property / content attribute would be better than a classname here.

listitem.hidden = true;

(Joe and Timothy may disagree for historical or project-specific reasons. If so, defer to their judgement.)

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:332
> +                    var moreNodes = document.createElement("button");

Even though this is functionally a <button>, it does not have to look like a push button. I would give this a general classname (e.g. not "node" specific) and some CSS styles that could be reused for other long lists in inspector panels.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:336
> +                            value.classList.remove("hidden");

I think the HTML5 @hidden DOM property / content attribute would be better than a classname here.

value.hidden = false;

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:343
> +            return hasLinks ? container : null;

I think I'd rather see container defined and returned inside the (nodeIds !== undefined) conditional.

        if (hasLinks)
            return container;
    }
    return null;

The fallback null return is likely to be extraneous at the end. I would usually expect to see this at the beginning of the function as an early return.

    function linkListForNodeIds(nodeIds) {
        if (!nodeIds) // Be more explicit than (!nodeIds) if you prefer.
            return;
Comment 19 Aaron Chu 2015-12-06 16:19:33 PST
>>Even though this is functionally a <button>, it does not have to look like a push button. I would give this a general classname (e.g. not "node" specific) and some CSS styles that could be reused for other long lists in inspector panels.

Would it be appropriate to style it so that it looks just like an element of the link list?
Comment 20 James Craig 2015-12-06 16:52:00 PST
(In reply to comment #19)
> Would it be appropriate to style it so that it looks just like an element of
> the link list?

I'm not sure. It's probably fine to start that way for the purposes of this patch. We can file a new bug if the usability of the "7 More..." expander is not obvious.
Comment 21 Aaron Chu 2015-12-06 22:55:33 PST
Created attachment 266752 [details]
Patch
Comment 22 James Craig 2015-12-07 03:23:39 PST
Comment on attachment 266752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266752&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:306
> +            if (!nodeIds) return null; 

WebKit Style: This should be on two lines without braces. You can run the check-webkit-style script before submitting and it will catch most of the style guide violations. 

    if (!nodeIds)
        return null;

Also review review https://webkit.org/code-style-guidelines/

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:344
> +            if (hasLinks && container) {
> +                return container;
> +            }

WebKit Style: no braces on single line conditionals. Run check-webkit-style and review https://webkit.org/code-style-guidelines/

> Source/WebInspectorUI/UserInterface/Views/Main.css:295
> +.node-link-list li[hidden=true],

*If* this rule needs to be defined, it should just be:

    [hidden],

or 

    *[hidden],

But you likely don't need this extra selector at all. Behavior for @hidden is in the default user agent style sheet.
Comment 23 James Craig 2015-12-07 03:26:15 PST
The rest looks good to me. You'll need the final r+ from JoePeck or Timothy.
Comment 24 Joseph Pecoraro 2015-12-07 11:34:54 PST
Comment on attachment 266752 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266752&action=review

My comments are almost entirely about Style. The patch itself looks fine to me though but I'd like to see another revision addressing the style comments!

Our JavaScript style guide is not exactly set in stone anyways but we try to update this wiki page and follow the main WebKit style guide:
http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide
https://webkit.org/code-style-guidelines/

> Source/WebInspectorUI/ChangeLog:12
> +
> +

Unnecessary blank line in here.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:316
> +            for (var nodeId of nodeIds) {
> +                var node = WebInspector.domTreeManager.nodeForId(nodeId);
> +                if (node) {
> +                    var link = WebInspector.linkifyAccessibilityNodeReference(node);
> +                    if (link) {

Style: Use `let` instead of `var` where possible.

This would read much easier with less nesting, using `continue`.

    for (let nodded of nodeIds) {
        let node = WebInspector.domTreeManager.nodeForId(nodeId);
        if (!node)
            continue;

        let link = WebInspector.linkifyAccessibilityNodeReference(node);
        // NOTE: This should not return null so no need to check for it.
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:318
> +                        var listitem = document.createElement("li");

Style: Variable names should be camel case. "listitem" => "listItem", or just "li" here.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:321
> +                            listitem.setAttribute("hidden", true);

Style: The hidden attribute has exposed DOM API. So you can just do: "listItem.hidden = true;"

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:332
> +                moreNodes.className += " expand-list-button";

Style: Prefer the classList DOM API: "moreNodes.classList.add("expand-list-button");

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:333
> +                moreNodes.innerText = WebInspector.UIString("%d More\u2026").format(listItemCount - itemsToShow);

Style: Prefer "textContent" over "innerText". "moreNodes.textContent = ..."

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:339
> +                moreNodes.addEventListener("click", function (evt) {
> +                    initiallyHiddenItems.forEach(function (value, index) {
> +                        value.setAttribute("hidden", false);
> +                    });
> +                    container.removeChild(this);
> +                });

Style: No space between and parenthesis for anonymous functions. "function (...)" => "function(...)"
Style: Prefer unabbreviated variable names for clarity: "evt" => "event"

Also you can take advantage of arrow functions here:

    moreNodes.addEventListener("click", function(event) {
        initiallyHiddenItems.forEach((element) => { element.hidden = false; })
        moreNodes.remove();
    });

>> Source/WebInspectorUI/UserInterface/Views/Main.css:295
>> +.node-link-list li[hidden=true],
> 
> *If* this rule needs to be defined, it should just be:
> 
>     [hidden],
> 
> or 
> 
>     *[hidden],
> 
> But you likely don't need this extra selector at all. Behavior for @hidden is in the default user agent style sheet.

Yes, default behavior should just work for this, you shouldn't need the new CSS here.

> Source/WebInspectorUI/UserInterface/Views/Main.css:302
> +    appearance: none;

Since Web Inspector in included in WebKit and we only target WebKit, we only need "-webkit-appearance" right now and not the maybe future available unprefixed version. If/when WebKit supports an unprefixed version we can update all the instances in Web Inspector's CSS. So lets just have "-webkit-appearance: none" here.

> ManualTests/accessibility/collapsible-node-link-list.html:11
> +<li>If the click on the said link does not functoin as described in #4, the test fails.</li>

Typo: "functoin" => "function"
Comment 25 Aaron Chu 2015-12-07 23:09:55 PST
Created attachment 266844 [details]
Patch
Comment 26 Aaron Chu 2015-12-07 23:10:36 PST
It looks like the li in .node-link-list are styled using ".node-link-list li", which overrode the default behavior, which is under simply "li". I think it may make more sense to use the existing ".hidden" to hide these items.

I also checked out the style guide and repositioned some of the braces in a function.
Comment 27 James Craig 2015-12-08 00:32:29 PST
(In reply to comment #26)
> It looks like the li in .node-link-list are styled using ".node-link-list
> li", which overrode the default behavior, which is under simply "li". I
> think it may make more sense to use the existing ".hidden" to hide these
> items.

I'm not sure I understand. 

Are you saying display was overridden so that @hidden did not work as intended? If so, you can probably fix the existing selector to include :not([hidden]), or perhaps add the !important directive to the local stylesheet for hidden:

*[hidden] {
  display: none !important;
}

If the local styles are overriding the expected behavior of @hidden, there may be a CSS bug that the default user style sheet needs to include the previous rule with the !important directive.
Comment 28 BJ Burg 2015-12-27 14:50:37 PST
Comment on attachment 266844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266844&action=review

I think this patch is ready to go. James, please land this (adding yourself to the Reviewed By: line) when you get back if there are no other issues.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:-276
> -        function booleanValueToLocalizedStringIfTrue(property) {

Either way is fine, you don't need to change this, but it's ok.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:311
> +                return null; 

Nit: usually add a newline after early bail or continue. Not a big deal though.
Comment 29 Aaron Chu 2016-01-07 19:16:52 PST
I believe I should take a look at the CSS [hidden] issue as pointed out by James.
Comment 30 Joseph Pecoraro 2016-01-07 19:49:18 PST
> *[hidden] {
>   display: none !important;
> }

We won't want to override the default style. The HTML spec recommends a default html stylesheet containing:

> [hidden], area, base, basefont, datalist, head, link, menu[type=context i], meta,
> noembed, noframes, param, rp, script, source, style, template, track, title {
>   display: none;
> }

https://html.spec.whatwg.org/multipage/interaction.html#the-hidden-attribute
https://html.spec.whatwg.org/multipage/rendering.html#hiddenCSS
Comment 31 Aaron Chu 2016-01-07 19:50:51 PST
Thanks Joe, I will look into :not([hidden]) and fix accordingly.
Comment 32 Devin Rousso 2016-01-07 21:02:51 PST
Comment on attachment 266844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266844&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:310
> +            if (!nodeIds) 

You could put this at the very beginning of the function, so that if it is true no variables are declared.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:312
> +            let linkList = document.createElement("ul");

NIT: What you have here is fine, but we have a prototype function on Element called createChild that accepts an element type and optional className, so this line would instead be (and you could then delete lines 313 and 334):
let linkList = container.createChild("ul", "node-link-list");

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:324
> +                    let li = document.createElement("li");

NIT: Same as above.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:336
> +                let moreNodes = document.createElement("button");

NIT: Same as above.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:340
> +                    initiallyHiddenItems.forEach((element) => { element.classList.remove("hidden"); });

NIT: You don't need the extra brackets after the "=>".

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:345
> +            if (hasLinks && container) 

I don't think you need to check for container being truthy as well, as it is always set on line 314.  Also, I would put a "return null;" after this if statement on the off-chance that some other circumstance arises where "hasLinks" is false for some reason.
Comment 33 James Craig 2016-01-08 01:49:03 PST
(In reply to comment #30)
> > *[hidden] {
> >   display: none !important;
> > }
> 
> We won't want to override the default style. The HTML spec recommends a
> default html stylesheet containing:
> 
> > [hidden], area, base, basefont, datalist, head, link, menu[type=context i], meta,
> > noembed, noframes, param, rp, script, source, style, template, track, title {
> >   display: none;
> > }
> 
> https://html.spec.whatwg.org/multipage/interaction.html#the-hidden-attribute
> https://html.spec.whatwg.org/multipage/rendering.html#hiddenCSS

I think that's likely a spec error. The behavior of @hidden is such that it should have a higher default specificity than simple attribute selector. I've raised it separately as bug 152883.
Comment 34 James Craig 2016-01-08 02:03:02 PST
Comment on attachment 266844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266844&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:277
> -        function booleanValueToLocalizedStringIfTrue(property) {
> +        function booleanValueToLocalizedStringIfTrue(property) 
> +        {

Don't include unrelated stylistic changes in a patch.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:284
> -        function booleanValueToLocalizedStringIfPropertyDefined(property) {
> +        function booleanValueToLocalizedStringIfPropertyDefined(property) 
> +        {

Ditto.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:295
> -        function linkForNodeId(nodeId) {
> +        function linkForNodeId(nodeId) 
> +        {

Ditto.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:345
>> +            if (hasLinks && container) 
> 
> I don't think you need to check for container being truthy as well, as it is always set on line 314.  Also, I would put a "return null;" after this if statement on the off-chance that some other circumstance arises where "hasLinks" is false for some reason.

I gave similar feedback on an earlier patch in comment #18.
Comment 35 Aaron Chu 2016-01-11 01:40:20 PST
Created attachment 268680 [details]
Patch
Comment 36 Devin Rousso 2016-01-12 00:13:16 PST
Comment on attachment 268680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268680&action=review

> Source/WebInspectorUI/ChangeLog:22
> +2016-01-11  Aaron Chu  <arona.chu@gmail.com>

Looks like you have two changelogs.  Merge them.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:310
> +            container.className = "list-container";

NIT: Use classList instead of className:
container.classList.add("list-container");

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:318
> +                if (link) {

`link` will not be null unless `node` is null, which is checked for on line 315, meaning that `link` will never be null.  As such you can remove this if statement.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:323
> +                        li.setAttribute("hidden", "");

NIT: Just doing `li.hidden = true;` would be valid as well.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:329
> +            container.appendChild(linkList);

This can be removed as createChild already does it.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:331
> +                let moreNodes = container.createChild("button");

You can make use of the optional className parameter of createChild:
let moreNodes = container.createChild("button", "expand-list-button");

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:343
> +

Remove extra newline.
Comment 37 James Craig 2016-01-12 01:32:05 PST
Comment on attachment 268680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268680&action=review

>> Source/WebInspectorUI/ChangeLog:22
>> +2016-01-11  Aaron Chu  <arona.chu@gmail.com>
> 
> Looks like you have two changelogs.  Merge them.

If I need to update a patch, I usually copy the comment, revert my changelogs, and rerun the prepare-Changelog script.

>> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:329
>> +            container.appendChild(linkList);
> 
> This can be removed as createChild already does it.

Ditto for line 321, (link in li)
Comment 38 Aaron Chu 2016-01-15 21:08:41 PST
Created attachment 269146 [details]
Patch
Comment 39 Joseph Pecoraro 2016-01-19 12:04:00 PST
Comment on attachment 269146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269146&action=review

The patch looks good to me. Please address the CSS issue and then we can look at landing it.

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:322
> +                    li.setAttribute("hidden", "");

This can just be:

    li.hidden = true;

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:329
> +                let moreNodes = container.createChild("button", "expand-list-button");

This variable name sounds like a list. How about naming it "moreNodesButton".

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:332
> +                    initiallyHiddenItems.forEach((element) => element.removeAttribute("hidden"));

And this can be:

    (element) => { element.hidden = false; }

> Source/WebInspectorUI/UserInterface/Views/Main.css:308
> +.expand-list-button {

When I applied this patch, clicking the "# More..." link caused the text of the link to go white.

This was unlike the action of clicking any of the other text links (like an individual node in the list above it).

It is changing because of the user agent style:

    button:active {
        color: activebuttontext;
    }

I think this would be preferred if the color stayed the same when active, so you could just add to these styles:

    .expand-list-button {
        color: black;
    }
Comment 40 Aaron Chu 2016-01-19 23:10:45 PST
Created attachment 269345 [details]
Patch
Comment 41 Joseph Pecoraro 2016-01-20 10:54:48 PST
Comment on attachment 269345 [details]
Patch

r=me
Comment 42 WebKit Commit Bot 2016-01-20 14:09:30 PST
Comment on attachment 269345 [details]
Patch

Rejecting attachment 269345 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 269345, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
oseph Pecoraro']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Parsed 6 diffs from patch file(s).
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 283.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Joseph Pecoraro']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/718603
Comment 43 Aaron Chu 2016-01-20 22:13:12 PST
noob here, what does this rejection mean? Is this because of the Loc file?
Comment 44 James Craig 2016-01-21 01:08:19 PST
(In reply to comment #43)
> noob here, what does this rejection mean? Is this because of the Loc file?

Looks like commit bot could not auto-merge some of the diffs in the strings file. Update your local SVN and manually merge that file before recreating the patch.
Comment 45 Aaron Chu 2016-01-23 17:46:16 PST
Created attachment 269680 [details]
Patch
Comment 46 WebKit Commit Bot 2016-01-23 18:47:53 PST
Comment on attachment 269680 [details]
Patch

Clearing flags on attachment: 269680

Committed r195512: <http://trac.webkit.org/changeset/195512>
Comment 47 WebKit Commit Bot 2016-01-23 18:47:58 PST
All reviewed patches have been landed.  Closing bug.