109291 - document.*Color is empty

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Henrik Gemal]

on http://gemal.dk/browserspy/showprop.html?document the property
"document.vlinkColor" is empty while with IE it's #800080

build 20011108

Comment 1

[Henrik Gemal]

the same goes for "document.linkColor" "document.fgColor" and "document.alinkColor"

Comment 2

[Johnny Stenback (:jst)]

Pushing to mozilla1.0

Comment 3

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 4

[Håkan Waara]

OK, I implemented:

* document.fgColor
* document.vlinkColor
* document.alinkColor
* document.linkColor

and those on the document.body.* to work like IE and NS4.x's attributes work,
when there attributes are not explicitely set.

Since neither the backend nor the UI supports getting/setting ALINKs of the UA
stylesheet, I will spin off a separate bug for that and for now make body.aLink
point to body.link (this is how IE works presently, AFAICT).

Comment 5

[Håkan Waara]

Attachment: Patch v1

Comments, suggestions, nits and reviews are all welcome.

Comment 6

[Håkan Waara]

Spun off bug 142163 for the alink issue.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

You don't need to call FlushPendingNotifications to flush reflows, since the
colors don't depend on reflows.

Comment 8

[Fabian Guisset]

Comment on attachment 82283 [details] [diff] [review]
Patch v1

Great patch, thanks for doing it! A few stylistic nits :-)

>+NS_IMETHODIMP
>+nsHTMLBodyElement::GetVLink(nsAString& aVlinkColor)
>+{
>+  // If we don't have an attribute, find the default color from the
>+  // UA stylesheet.
>+  if (NS_CONTENT_ATTR_NOT_THERE == GetAttr(kNameSpaceID_None, nsHTMLAtoms::vlink, aVlinkColor)) {

Please move the assignment out of the if. Do something like this:
nsresult rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::vlink, aVlinkColor);
if (rv == NS_CONTENT_ATTR_NOT_THERE)

(we use rv instead of result everywhere)

>+    nsresult result = NS_OK;
>+    if (mDocument) {
>+      // Make sure the presentation is up-to-date
>+      result = mDocument->FlushPendingNotifications();
>+      if (NS_FAILED(result)) {
>+        return result;
>+      }
>+    }
>+
What dbaron said. Also please use NS_ENSURE_SUCCESS(rv, rv) everywhere instead
of
if (NS_FAILED(rv)) {
  return rv;
}

>+    nsCOMPtr<nsIPresContext> context;
>+    result = GetPresContext(this, getter_AddRefs(context));
>+
>+    if (NS_FAILED(result)) {
>+      return result;
>+    }
>+
>+    nscolor vlinkColor;
>+    result = context->GetDefaultVisitedLinkColor(&vlinkColor);
>+
You don't use the value of result here. Please use it or remove it.

Apart from those nits, it's great! There are even comments, wow :-)

Comment 9

[Håkan Waara]

Is it safe to fix all these nits (FlushNotifications and stylistc) in
GetBgColor() as well, or is that somehow special-cased?

Comment 10

[Håkan Waara]

Looks like it depends on the FlushPendingNotifications() call since it checks
the primary frame.

Comment 11

[Fabian Guisset]

Yep I'd say clean up GetBgColor as well, it's old code :-) Not sure about
FlushPendingNotifications for that function, though.

Comment 12

[Håkan Waara]

Attachment: Patch v2

Changes:

* GetBgColor() stylistically cleaned up.
* Dbaron's and Fabian's nits fixed.

Comment 13

[Fabian Guisset]

Comment on attachment 82324 [details] [diff] [review]
Patch v2

r=fabian

Comment 14

[Håkan Waara]

Reassigning.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 82324 [details] [diff] [review]
Patch v2

Ignore the return value from GetPresContext() and just check for a non-null
prescontext and return NS_OK if there's no prescontext. Not having a
prescontext is not an error case so we don't want to throw an exception in such
a case.

sr=jst with that change (upon checkin).

Comment 16

[Håkan Waara]

Attachment: Patch v3

Final patch with jst's comments incorporated.

Comment 17

[Håkan Waara]

Fix checked in.

Comment 18

[Håkan Waara]

Marking fixed too.