Bug 30079 - unselectable resources in resource panel
Summary: unselectable resources in resource panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-05 09:57 PDT by Patrick Mueller
Modified: 2009-10-08 15:21 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch 2009/10/07 - a (3.66 KB, patch)
2009-10-07 09:00 PDT, Patrick Mueller
no flags Details | Formatted Diff | Diff
proposed patch 2009/10/08 - a (5.10 KB, patch)
2009-10-08 13:40 PDT, Patrick Mueller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Mueller 2009-10-05 09:57:29 PDT
See Bug 28812, comment 6.

In the resources panel, there are cases where some resources are unselectable.  For example, load the URL:

   http://www.lumacentral.com/CocaCola/index.html

and you should see two resources named "execBatch".  Only one is selectable.  The other, when you click on it, quickly deselects itself and the selects the other one.

Given the circumstances, it smells like the problem is that this is the case with resources with the same URL and method.  In the example described, the resource is essentially an RPC service, where the only difference between the requests is the HTTP request payload.
Comment 1 Patrick Mueller 2009-10-06 08:15:11 PDT
It's possible to duplicate this behavior with the following HTML

----- snip here -----
testing

<script>

var xhr;

xhr = new XMLHttpRequest();
xhr.open("POST", "http://example.org/abc", false);
xhr.send("123");

xhr = new XMLHttpRequest();
xhr.open("POST", "http://example.org/abc", false);
xhr.send("456");

xhr = new XMLHttpRequest();
xhr.open("POST", "http://example.org/something.else", false);
xhr.send("789");

</script>
----- snip here -----

The results are somewhat intermittent; sometimes you get the can't select behavior, sometimes you can; usually you can.  If you don't immediately see it, reload the URL in the browser with WI still up.  In those cases, you will typically see the "no content" condition of Bug 30080 as well.

Smells like the problem is multiple requests with the same method/url.
Comment 2 Jeroen Bensch 2009-10-06 09:21:54 PDT
(In reply to comment #1)

You can _always_ select the first "abc" by clicking _on_ the "abc" text for the first one and dragging slightly upwards, or clicking anywhere in the first "abc" and drag until the overlay appears that appears when attempting to drag it out of the window...

So this could be a simple missing "resourcesList.selectedItem" (I don't know) update when clicking it... something trivial GUI-like?
Comment 3 Patrick Mueller 2009-10-06 11:28:20 PDT
If you look at a trace of the relevant methods invoked, it turns out everything pretty much goes as planned, but then WebInspector.documentClick() is invoked because the item in the Resources list is actually living in an <a> element.  You can see in that code that what ends up happening is that WebInspector.showResourceForURL() is invoked.  That method only takes the href as a parameter.  At that point, all hell breaks loose.

In theory then, this would break if you had an XHR with a "GET" followed by a "DELETE" as well, since the href would be the same for them.

Seems to me like the WebInspector.documentClick() functionality isn't needed here.  And that the <a> in the item in the Resources isn't really needed; clicking on those items is handled by the tree code itself.

So, I did the simplest possible thing.  In WebInspector.ResourceSidebarTreeElement.prototype.onattach(), I changed the following code:

   var link = document.createElement("a");
   link.href = this.resource.url;

to:

   var link = document.createElement("span");

to see what would happen.  This changes the <a> element to a <span>, so WebInspector.documentClick() isn't invoked anymore.

Voila!  Now it works.  

The question is, what did this break?  Is anyone else dependent on the Resources elements list?  If not, a nicer looking modification than what I just did should suffice as a fix.  Presumably change the name of the var from "link" to "span" or "element".
Comment 4 Patrick Mueller 2009-10-06 12:23:24 PDT
Given what I know about this, another way it can get screwed up, even after removing the link, would be if you could somehow click an actual link to something - say an <a> element in an Elements display, which somehow pointed back to a resource with the same URL as the one you were looking at, but was actually a different resource.  I've been unable to duplicate this with some simple tests.  Probably something you wouldn't see very often.  

An example might be something like an Atom feed.  Say you had some XHR code that was dealing with feeds, and those XHRs were accessing the feed resource itself.  Feeds often refer to themselves with 'self' sort of link.  

Wondering if there are other edge cases like this we might run into.  Again, doesn't seem like something you'd run into very often, so I'm not worried about it right now.
Comment 5 Patrick Mueller 2009-10-07 09:00:17 PDT
Created attachment 40791 [details]
proposed patch 2009/10/07 - a

Removed apparently vestigal code.

The ResourceSidebarTreeElements were being created by creating a new <a> element, and moving all the children in the existing tree element into the new <a> element, then making the <a> element the only child of the tree element.  Basically, wrapping the existing tree element content in an <a> element.

The <a> element isn't needed, because selection of the tree element is already handled by the tree element itself.  The <a> element click handling was being processed by WebInspector.documentClick(), which was then selecting a potentially incorrect resource, if multiple resources with the same URL exist in the resource tree.

The "invisible" CSS class is no longer used by any code in Web Inspector, as near as I can tell (search through inspector code).

Test case was added with three resource retrievals of the same URL, but different sorts of invocation - two POSTs with different content, and a GET.  Without the fix, only one of the three resources was selectable in the UI.
Comment 6 Timothy Hatcher 2009-10-07 10:15:27 PDT
Comment on attachment 40791 [details]
proposed patch 2009/10/07 - a

So this reverts the earlier change Joe made to allow dragging resources?
Comment 7 Patrick Mueller 2009-10-07 10:46:18 PDT
(In reply to comment #6)
> (From update of attachment 40791 [details])
> So this reverts the earlier change Joe made to allow dragging resources?

Ahh ... woops ... it would in fact revert Bug 14410.  Which we don't want to do, I assume.

hmmm ... the other option, assuming we can't somehow fix this within the tree element itself, is to nullify the call to WebInspector.showResourceForURL() that happens in WebInspector.documentClick().  

We could add an additional class to the <a> element called "ignoreClickEvent" which we could then check for in documentClick() and ignore those clicks.

Not quite sure what's going on with the webkit-html-external/resource-link styles - perhaps there's something we can do there as well.

Should add a comment to the bit in ResourceSidebarTreeElement indicating why the <a> element is being created in the first place.
Comment 8 Timothy Hatcher 2009-10-07 11:07:50 PDT
We could reimplement Bug 14410 usign HTML5 drag and drop and not need an <a>.
Comment 9 Patrick Mueller 2009-10-07 11:55:38 PDT
(In reply to comment #8)
> We could reimplement Bug 14410 usign HTML5 drag and drop and not need an <a>.

I'm game for that.  I have a patch now that adds a property to the <a> element which gets checked in the document's click handler and then ignores the click.  Super easy and safe.  But if we want to do the DnD, I won't bother; the bug isn't critical, and there is a work-around described in comment 2.
Comment 10 Timothy Hatcher 2009-10-07 12:37:36 PDT
Drag and drop would be the most elegant fix.
Comment 11 Patrick Mueller 2009-10-08 13:40:02 PDT
Created attachment 40908 [details]
proposed patch 2009/10/08 - a

The ResourceSidebarTreeElements were being created by creating a new <a>
element, and moving all the children in the existing tree element into the new
<a> element, then making the <a> element the only child of the tree element. 
Basically, wrapping the existing tree element content in an <a> element.

The <a> element isn't needed, because selection of the tree element is already
handled by the tree element itself.  The <a> element click handling was being
processed by WebInspector.documentClick(), which was then selecting a
potentially incorrect resource, if multiple resources with the same URL exist
in the resource tree.

The "invisible" CSS class is no longer used by any code in Web Inspector, as
near as I can tell (search through inspector code).

The reason the <a> element was added was to allow the resource elements to
be dragged elsewhere and rendered on the drop target as the URL to resource.
Removing the <a> element nullifies that ability.  Code was added to use HTML5
DnD to perform the same function.

Test case was added with three resource retrievals of the same URL, but
different sorts of invocation - two POSTs with different content, and a GET. 
Without the fix, only one of the three resources was selectable in the UI.
Comment 12 WebKit Commit Bot 2009-10-08 13:57:34 PDT
Comment on attachment 40908 [details]
proposed patch 2009/10/08 - a

Clearing flags on attachment: 40908

Committed r49315: <http://trac.webkit.org/changeset/49315>
Comment 13 WebKit Commit Bot 2009-10-08 13:57:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Joseph Pecoraro 2009-10-08 14:02:04 PDT
Nice solution!

There was a style slip on:

>   ondragstart: function(event) {

The brace should be on the next line.
Comment 15 Patrick Mueller 2009-10-08 15:20:47 PDT
(In reply to comment #14)

Good catch.  Presumably we'll be back in this code for the FIXME.  Should we wait to fix it then?
Comment 16 Timothy Hatcher 2009-10-08 15:21:33 PDT
Yes just leave it for now.