344955 - Cross-Synchronize Toolkit and XPFE <consoleBindings.xml>

Status: RESOLVED WONTFIX

(Core Graveyard :: Error Console, defect)

Description

[Serge Gautherie (:sgautherie)]

2 from XPFE: bug 288544 bug 290429
3 from Toolkit: bug 301398 bug 302402 bug 306223

Comment 1

[Serge Gautherie (:sgautherie)]

1 more from XPFE: bug 179268

Comment 2

[Serge Gautherie (:sgautherie)]

1 more from Toolkit: bug 289927 ... Done there.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-XPFE-Bw) Bug 301398 port

Bug 301398 port,
as it was checked in [focus border color is '-moz-DialogText' instead of 'black' initialy];
This '-Bw' patch is for review only.

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1b1) Gecko/20060717 SeaMonkey/1.1a] (nightly) (W98SE)

I did a minimal test with this build in Classic theme, which seems to be working well...

Comment 4

[[e-mail]]

Comment on attachment 229934 [details] [diff] [review]
(Av1-XPFE-Bw) Bug 301398 port

The CSS changes look good although I haven't tried them yet.

>-      <xul:label class="text-link" xbl:inherits="value=url" crop="right"/>
>+      <xul:label class="text-link" xbl:inherits="href,value=href" crop="right"/>
I don't understand the attribute name change here - this isn't a "real" text link because we open the view source window, so it doesn't need an href.

>-label.text-link {
>+label.text-link, label[onclick] {
I don't think this change is correct at all.

Comment 5

[Serge Gautherie (:sgautherie)]

CC'ing Aaron, who, I hope, could answer your questions as Toolkit patch author,
because I obviously can't.

Comment 6

[Serge Gautherie (:sgautherie)]

Bug 306223 was ported to SeaMonkey there.

Comment 7

[[e-mail]]

Can you do a patch just with the outline CSS changes? Thanks.
(AFAIK outline: invert; doesn't work properly in Cairo builds.)

Comment 8

[Serge Gautherie (:sgautherie)]

With:
Only the 3 <formatting.css> files !?
Only the 3 |.text-link:focus| rules changes !?

Comment 9

[Aaron Leventhal]

(In reply to comment #4)
> >-      <xul:label class="text-link" xbl:inherits="value=url" crop="right"/>
> >+      <xul:label class="text-link" xbl:inherits="href,value=href" crop="right"/>
> I don't understand the attribute name change here - this isn't a "real" text
> link because we open the view source window, so it doesn't need an href.
We use the href in nsXULTextAccessible.cpp in 2 places. We use it to expose the accessible value of the XUL link, which should be the URL. We also use it to know which element should receive a click when the link is supposed to be activated (some XUL links are unfortunately created with a pair of elements, a parent and a child).

> >-label.text-link {
> >+label.text-link, label[onclick] {
> I don't think this change is correct at all.
This change makes it so that when a XUL author places an onclick on a XUL label, it will still be accessible. The appearance isn't being changed here, but the way it is exposed to accessibility is. It should be exposed as a link or a button -- something actionable, and needs to be tabbable. If we don't do this we keep ending up with inaccessible XUL clickable labels.

Comment 10

[[e-mail]]

OK, but then this patch is just missing the change to consoleBindings.xml that makes it only open the view source window and not the original URL.

Comment 11

[Philip Chee]

Trunk is now using the toolkit Error Console. Do we need this for branch?

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> 1 more from XPFE: bug 179268

Neil,
Should "I" port your <consoleBindings.xml> patch there to </toolkit/> ?

Comment 13

[[e-mail]]

(In reply to comment #12)
> (In reply to comment #1)
> > 1 more from XPFE: bug 179268
> Neil,
> Should "I" port your <consoleBindings.xml> patch there to </toolkit/> ?
If they want it, then feel free to port it.

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 229934 [details] [diff] [review]
(Av1-XPFE-Bw) Bug 301398 port

patch expired/bitrotted

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #11)
> Trunk is now using the toolkit Error Console. Do we need this for branch?

Yes this seems done.. EXPIRED; reopen if I'm wrong.(