Bug 130911 – Web Inspector: AXI: node-link-list should be collapsible

• Component: Web Inspector
• Status: RESOLVED FIXED

[James Craig]

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.

[James Craig]

Created attachment 228079 [details]
screen shot

[Radar WebKit Bug Importer]

<rdar://problem/16460250>

[Timothy Hatcher]

This is making me wonder if Children is a good thing to include in the AX properties.

[James Craig]

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.

[James Craig]

(Chris's patch being bug 130563.)

[James Craig]

W3C specs are another place where you can get giant lists of AX Children.

[Timothy Hatcher]

I think making it collapsed with a "More…" inline link at the end would be a good start.

[Joseph Pecoraro]

(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..."

[James Craig]

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.

[Timothy Hatcher]

5 sounds good. Just More… and clicking away to another DOM node seems fine.

[Aaron Chu]

Created attachment 266430 [details]
Patch

[Aaron Chu]

Created attachment 266432 [details]
Patch

[James Craig]

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.

[James Craig]

Please forgive all the typos in my comments. It's 3am. Calling it a night. ;-)

[Joseph Pecoraro]

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.

[James Craig]

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.

[Aaron Chu]

Created attachment 266739 [details]
Patch

[James Craig]

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;

[Aaron Chu]

>>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?

[James Craig]

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

[Aaron Chu]

Created attachment 266752 [details]
Patch

[James Craig]

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.

[James Craig]

The rest looks good to me. You'll need the final r+ from JoePeck or Timothy.

[Joseph Pecoraro]

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"

[Aaron Chu]

Created attachment 266844 [details]
Patch

[Aaron Chu]

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.

[James Craig]

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

[Blaze Burg]

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.

[Aaron Chu]

I believe I should take a look at the CSS [hidden] issue as pointed out by James.

[Joseph Pecoraro]

> *[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

[Aaron Chu]

Thanks Joe, I will look into :not([hidden]) and fix accordingly.

[Devin Rousso]

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.

[James Craig]

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

[James Craig]

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.

[Aaron Chu]

Created attachment 268680 [details]
Patch

[Devin Rousso]

Comment on attachment 268680 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:22
> [phone]  Aaron Chu  <[e-mail]>

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.

[James Craig]

Comment on attachment 268680 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:22
>> [phone]  Aaron Chu  <[e-mail]>
> 
> 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)

[Aaron Chu]

Created attachment 269146 [details]
Patch

[Joseph Pecoraro]

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;
    }

[Aaron Chu]

Created attachment 269345 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 269345 [details]
Patch

r=me

[WebKit Commit Bot]

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

[Aaron Chu]

noob here, what does this rejection mean? Is this because of the Loc file?

[James Craig]

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

[Aaron Chu]

Created attachment 269680 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 269680 [details]
Patch

Clearing flags on attachment: 269680

Committed r195512: <http://trac.webkit.org/changeset/195512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.